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

make SchedResourceList optional in flux.job.info #5141

Conversation

vsoch
Copy link
Member

@vsoch vsoch commented May 3, 2023

Problem: We currently cannot package rlist.h with the install (at least without more work) and since this is a required import for flux.job.info, without it the Python bindings (installed via pip) will error.
Solution: the flux.resource module is not technically required, so we can fallback to catching the ImportError instead and using the same empty (dummy) class to handle self.resources

I am going to test this with the Python bindings, and would like to get that working first before we finalize it here! Note that I'm going to be taking a break for a few minutes but I'll be back after. I'll update the issue after I have done some testing with this branch!

@vsoch
Copy link
Member Author

vsoch commented May 3, 2023

@grondo @garlick this is fully working! I think if we want to release older versions of flux for flux-python I can do some custom and manual work to make that happen (it's basically backporting this change, at least I just tested with 0.48.0 and it worked on corona) but if we can get this into a release, it should allow us to build python bindings from here on out. Tomorrow let's chat about if you want me to do the backported releases - my thinking is that before we do the official ones (because you can't undo/redo a release on pypi) we should do the following:

  • Decide on a set of python tests for flux-python (if you want to point me to relevant files in core I can figure it out)
  • Once those are working, find a user / small set of users to test a release candidate for some version they have

When those two things are done (it's working for the user and we have tests) I'll be comfortable doing "final" releases for older versions (with the manual work) and then I'll finish up the automated workflow for new releases. It's already written but I need to turn it on.

Copy link
Contributor

@grondo grondo left a comment

Choose a reason for hiding this comment

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

This seems like a good workaround for now, especially if it lets us move forward on the pip install of Python bindings. Thanks!

@vsoch
Copy link
Member Author

vsoch commented May 3, 2023

Thanks! And let me know when you want to discuss the plan above - likely y’all can do the release work first and come back to it. I’m planning on rebuilding my flux images with this new release too.

Problem: We currently cannot package rlist.h with the install (at
least without more work) and since this is a required import for
flux.job.info, without it the Python bindings (installed via pip)
will error.
Solution: the flux.resource module is not technically required,
so we can fallback to catching the ImportError instead and using
the same empty (dummy) class to handle self.resources

Signed-off-by: vsoch <vsoch@users.noreply.github.com>
@vsoch vsoch force-pushed the refactor/flux-resource-make-optional branch from 9d00bd9 to ed8b0ac Compare May 3, 2023 13:38
@codecov
Copy link

codecov bot commented May 3, 2023

Codecov Report

Merging #5141 (9d00bd9) into master (e79b7fd) will increase coverage by 0.00%.
The diff coverage is 60.00%.

❗ Current head 9d00bd9 differs from pull request most recent head ed8b0ac. Consider uploading reports for the commit ed8b0ac to get more accurate results

@@           Coverage Diff           @@
##           master    #5141   +/-   ##
=======================================
  Coverage   83.11%   83.11%           
=======================================
  Files         453      453           
  Lines       77750    77745    -5     
=======================================
- Hits        64625    64621    -4     
+ Misses      13125    13124    -1     
Impacted Files Coverage Δ
src/bindings/python/flux/job/info.py 94.13% <60.00%> (-0.54%) ⬇️

... and 5 files with indirect coverage changes

@mergify mergify bot merged commit ca19cb0 into flux-framework:master May 3, 2023
29 of 31 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants