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.

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

AsyncioDispatcher cleanup tasks atexit #138

Conversation

mdavidsaver
Copy link
Contributor

Currently, the atexit logic simply breaks the event loop leaving an in-progress Task incomplete. Instead, use asyncio.run(), which cancels tasks before returning (among other cleanup).

Motivated by usage of asyncio.create_subprocess_exec() where the present behavior leaves the child process running. Similarly, for cleanup of temporary files/directories.

The downside is that asyncio.run() is introduced in python 3.7. So the 3.6 tests will fail. Emulating asyncio.run() in 3.6 would be possible, but is it worth the effort?

I don't know what is going wrong with the windows and mac tests.

@codecov
Copy link

codecov bot commented Jul 16, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.15%. Comparing base (d55483d) to head (f7a1f53).

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #138      +/-   ##
==========================================
+ Coverage   87.73%   88.15%   +0.41%     
==========================================
  Files          14       14              
  Lines        1019     1038      +19     
==========================================
+ Hits          894      915      +21     
+ Misses        125      123       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@coretl coretl left a comment

Choose a reason for hiding this comment

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

I think it is worth ditching 3.6 so we get asyncio.run. @AlexanderWells-diamond @Araneidae what do you think? This looks ok to me, I asked a couple of questions out of interest. It need 3.6 ditching from the matrix before merging. The mac and windows tests seem to be working in the latest run?

softioc/asyncio_dispatcher.py Show resolved Hide resolved
softioc/asyncio_dispatcher.py Show resolved Hide resolved
@AlexanderWells-diamond
Copy link
Collaborator

I have no attachment to Python 3.6 so am happy to see it go.

@Araneidae
Copy link
Collaborator

@coretl, I also have no ties to 3.6 ... except I would point out that it is the stock version of Python installed on RHEL8, so dropping it may have consequences for users on that platform.

@mdavidsaver
Copy link
Contributor Author

mdavidsaver commented Jul 17, 2023

... stock version of Python installed on RHEL8 ...

This is a fair point. I can think of two possibilities in addition to drop support for 3.6 or a full backport of asyncio.run.

State that the asyncio part is only supported with py >= 3.7. (fyi. driven by removal of loop= P4P only supports asyncio with py >= 3.6, but supports thread/cothread with py 2.7 and >= 3.4)

Continue the current behavior with <= 3.6

@coretl
Copy link
Contributor

coretl commented Jul 24, 2023

My preference is to drop 3.6, bump the major release number, and see if anyone complains... Any objections?

@AlexanderWells-diamond
Copy link
Collaborator

No objections here as long as its clear in the Changelog.

@mdavidsaver
Copy link
Contributor Author

Is there anything I can do to move this PR forward?

@coretl
Copy link
Contributor

coretl commented Mar 4, 2024

Is there anything I can do to move this PR forward?

If you can drop 3.6 from the GitHub actions tests matrix, then add a test with your usecase (checking a spawned subprocess running as a Task exits cleanly outside context manager) then I'd be happy to merge this.

@AlexanderWells-diamond @Araneidae any thoughts?

@Araneidae
Copy link
Collaborator

I have learnt that there is a python3.11 rpm available on our RHEL8 channel and so presumably potentially available on all administered RHEL8 systems, so I agree that 3.6 can now go.

@AlexanderWells-diamond
Copy link
Collaborator

This PR will need some tidying up before we can merge it (remove 3.6, documentation, changelog etc.); I'll do that in the near future.

@AlexanderWells-diamond
Copy link
Collaborator

I've updated this PR to remove Python 3.6 support and added a Changelog entry.

@AlexanderWells-diamond AlexanderWells-diamond requested review from Araneidae and removed request for AlexanderWells-diamond March 19, 2024 09:42
@Araneidae Araneidae requested a review from EmilioPeJu March 19, 2024 11:09
@AlexanderWells-diamond
Copy link
Collaborator

With my addition of a test for the Context Manager, I'm finished with my changes. @Araneidae and/or @coretl please merge when you are happy with it.

@mdavidsaver
Copy link
Contributor Author

@AlexanderWells-diamond Thank you for taking the time to finish this up!

@AlexanderWells-diamond AlexanderWells-diamond merged commit a89d8ae into DiamondLightSource:master Apr 2, 2024
31 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants