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

reset modules loaded by PythonPackage to let ExtensionEasyBlock handle multi_deps correctly #2951

Merged
merged 1 commit into from
Jun 20, 2023
Merged

reset modules loaded by PythonPackage to let ExtensionEasyBlock handle multi_deps correctly #2951

merged 1 commit into from
Jun 20, 2023

Conversation

mboisson
Copy link
Contributor

@mboisson mboisson commented Jun 19, 2023 •
edited by boegel
Loading

PythonPackage's sanity_check_step loads modules, but is unaware of the handling of multi_deps done by the ExtensionEasyBlock version of sanity_check_step.

This PR unloads the modules loaded during PythonPackage's version to let ExtensionEasyBlock handle multi_deps correctly.

@mboisson mboisson mentioned this pull request Jun 19, 2023
Merged
@mboisson
Copy link
Contributor Author

This fixes a regression introduced by #2745

@ocaisa
Copy link
Member

ocaisa commented Jun 20, 2023

This looks narrow in scope enough to be ok to merge. Since we don't have test suites for this and there are wide base of use cases and configurations, it's very hard to check this. You created the PR to work for installing a module, does it also work for --module-only for your case (which was what #2745 was fixing)?

@boegel boegel added this to the next release (4.7.3?) milestone Jun 20, 2023
@boegel boegel added the bug fix label Jun 20, 2023
@boegel boegel changed the title reset modules loaded by PythonPackage to let ExtensionEasyBlock handl… reset modules loaded by PythonPackage to let ExtensionEasyBlock handle multi_deps correctly Jun 20, 2023
@mboisson
Copy link
Contributor Author

Yes, it did work correctly with --module-only for our use case.

ocaisa
ocaisa approved these changes Jun 20, 2023
View reviewed changes
Copy link
Member

@ocaisa ocaisa left a comment

Choose a reason for hiding this comment

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

LGTM

@ocaisa ocaisa merged commit 03743de into easybuilders:develop Jun 20, 2023
@boegel
Copy link
Member

boegel commented Jun 21, 2023

Test report by @boegel

Overview of tested easyconfigs (in order)

  • SUCCESS tqdm-4.47.0-GCCcore-9.3.0.eb
  • SUCCESS BeautifulSoup-4.9.1-GCCcore-8.3.0.eb

Build succeeded for 2 out of 2 (2 easyconfigs in total)
node3571.doduo.os - Linux RHEL 8.6, x86_64, AMD EPYC 7552 48-Core Processor (zen2), Python 3.6.8
See https://gist.github.com/boegel/613dc7c17ba5b7bb1f7fd5f9dccbeed2 for a full test report.

@boegel
Copy link
Member

boegel commented Jun 21, 2023 •
edited
Loading

Test report by @boegel

Overview of tested easyconfigs (in order)

  • SUCCESS tqdm-4.47.0-GCCcore-9.3.0.eb
  • SUCCESS BeautifulSoup-4.9.1-GCCcore-8.3.0.eb

Build succeeded for 2 out of 2 (2 easyconfigs in total)
node3571.doduo.os - Linux RHEL 8.6, x86_64, AMD EPYC 7552 48-Core Processor (zen2), Python 3.6.8
See https://gist.github.com/boegel/4ce9b5bc0af1183be0e27a81695aedd3 for a full test report.

edit: test with --sanity-check-only

@boegel
Copy link
Member

boegel commented Jun 21, 2023

Test report by @boegel

Overview of tested easyconfigs (in order)

  • SUCCESS tqdm-4.47.0-GCCcore-9.3.0.eb
  • SUCCESS BeautifulSoup-4.9.1-GCCcore-8.3.0.eb

Build succeeded for 2 out of 2 (2 easyconfigs in total)
node3571.doduo.os - Linux RHEL 8.6, x86_64, AMD EPYC 7552 48-Core Processor (zen2), Python 3.6.8
See https://gist.github.com/boegel/6f15a30d9e8bff9e097d1cd4e37f41d4 for a full test report.

@mboisson mboisson deleted the fix_sanity_check_multi_deps_pythonpackage branch June 21, 2023 13:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ocaisa ocaisa ocaisa approved these changes

Assignees
No one assigned
Labels
bug fix
Projects
None yet
Milestone
4.8.0
Development

Successfully merging this pull request may close these issues.
3 participants
@mboisson @ocaisa @boegel