Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[#217,#218] Remove Python 2 support code, implement proper thread state handling (4-3-stable) #220

Merged
merged 2 commits into from
Aug 22, 2024
Merged

[#217,#218] Remove Python 2 support code, implement proper thread state handling (4-3-stable) #220

merged 2 commits into from
Aug 22, 2024

Conversation

SwooshyCueb
Copy link
Member

Addresses #217
Addresses #218 (WIP)

Not tested yet, could probably be improved.

Since I am currently testing with 4-3-stable, progress may be followed here instead of in #219.

@SwooshyCueb SwooshyCueb marked this pull request as draft August 19, 2024 17:11
@trel trel mentioned this pull request Aug 19, 2024
Merged
@SwooshyCueb SwooshyCueb force-pushed the 218.4-3-stable branch 2 times, most recently from 890b570 to 47b50cf Compare August 19, 2024 20:02
@SwooshyCueb SwooshyCueb marked this pull request as ready for review August 20, 2024 01:44
@SwooshyCueb
Copy link
Member Author

Ran the test suites for Ubuntu 22.04 and 24.04 and everything passed, so I'm marking this ready for review. Will squash and update the other PR tomorrow.

@alanking alanking requested a review from d-w-moore August 20, 2024 13:40
alanking
alanking reviewed Aug 20, 2024
View reviewed changes
Copy link
Contributor

@alanking alanking left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I defer to others. Had a couple of suggestions but looks fine overall. Glad it's working

src/main.cpp Outdated Show resolved Hide resolved
src/main.cpp Show resolved Hide resolved
@korydraughn
Copy link
Contributor

After some discussion, the plan is to see if testing passes for Ubuntu 20 before testing non-Ubuntu platforms.

If the tests pass for all platforms, then Ubuntu 24 and the PREP get a 4.3.3 release.

@SwooshyCueb
Copy link
Member Author

Tests pass for Ubuntu 20.04. Commits squashed. If there are no objections, I will #.

@korydraughn
Copy link
Contributor

Let's test the remaining platforms first just in case other tweaks are needed.

I'll pull your changes and start running tests.

@SwooshyCueb SwooshyCueb mentioned this pull request Aug 21, 2024
Draft
@SwooshyCueb SwooshyCueb changed the title [WIP] [#217,#218] Remove Python 2 support code, implement proper thread state handling (4-3-stable) [#217,#218] Remove Python 2 support code, implement proper thread state handling (4-3-stable) Aug 21, 2024
korydraughn
korydraughn reviewed Aug 21, 2024
View reviewed changes
Copy link
Contributor

@korydraughn korydraughn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good overall.

Once the review comments have been addressed, I think we'll be ready to get this in.

Good stuff.

src/main.cpp Outdated Show resolved Hide resolved
src/main.cpp Outdated Show resolved Hide resolved
src/main.cpp Show resolved Hide resolved
src/main.cpp Show resolved Hide resolved
src/main.cpp Outdated Show resolved Hide resolved
src/main.cpp Outdated Show resolved Hide resolved
src/main.cpp Outdated Show resolved Hide resolved
@korydraughn
Copy link
Contributor

Because python_thread_state_scope has changed (i.e. disabled copy/move semantics), please confirm test_icp.Test_Icp.test_multithreaded_icp__issue_5478 still passes with this PR.

I don't suspect anything to break, but we need to be sure.

@SwooshyCueb
Copy link
Member Author

Because python_thread_state_scope has changed (i.e. disabled copy/move semantics), please confirm test_icp.Test_Icp.test_multithreaded_icp__issue_5478 still passes with this PR.

It still passes. Tested on Ubuntu 24.04 and 20.04.

korydraughn
korydraughn reviewed Aug 21, 2024
View reviewed changes
Copy link
Contributor

@korydraughn korydraughn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent. Code is good/ready.

Please add words to the body of the commit message for #218 which explain what is being solved.

@SwooshyCueb
Copy link
Member Author

Please add words to the body of the commit message for #218 which explain what is being solved.

Done

korydraughn
korydraughn approved these changes Aug 21, 2024
View reviewed changes
Copy link
Contributor

@korydraughn korydraughn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pound it.

@SwooshyCueb
[irods#217] Remove Python 2 support code
4670d26
@SwooshyCueb
[irods#218] Proper python multithreading
94fbb04 
With Python 3.12, we started seeing crashes during certain multithreaded
operations. It turns out that changes to the GIL in 3.12 exposed that we are
not handling Python thread state properly. This commit fixes this.

A new namespace python_state has been added to main.cpp for holding onto some
Python state information, and a new struct python_thread_state_scope has also
been added for managing Python thread state.

This will be improved upon in the future.
@SwooshyCueb
Copy link
Member Author

#'d

@alanking alanking merged commit ef4ca71 into irods:4-3-stable Aug 22, 2024
1 of 2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alanking alanking alanking approved these changes

@korydraughn korydraughn korydraughn approved these changes

@d-w-moore d-w-moore Awaiting requested review from d-w-moore

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@SwooshyCueb @korydraughn @alanking