Skip to content

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

Merged
ocaisa merged 1 commit intoeasybuilders:developfrom
ComputeCanada:fix_sanity_check_multi_deps_pythonpackage
Jun 20, 2023
Merged

reset modules loaded by PythonPackage to let ExtensionEasyBlock handle multi_deps correctly#2951
ocaisa merged 1 commit intoeasybuilders:developfrom
ComputeCanada:fix_sanity_check_multi_deps_pythonpackage

Conversation

@mboisson
Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

mboisson commented Jun 19, 2023

This fixes a regression introduced by #2745

@ocaisa
Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

mboisson commented Jun 20, 2023

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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