Skip to content

fix double initialization of Cargo by CargoPythonPackage by removing incorrect custom __init__ implementation + fix use of super() in PALM easyblock (since that doesn't work with Python 2.7)#3406

Merged
boegel merged 2 commits intoeasybuilders:developfrom
Flamefire:fix-double-init
Aug 14, 2024

Conversation

@Flamefire
Copy link
Contributor

The issue intended to be fixed with #2934 is actually (likely) caused by Pythons MRO:

CargoPythonPackage explicitely calls __init__ of both Cargo and PythonPackage

  1. super().__init__ in Cargo calls ExtensionEasyBlock->EasyBlock stop (Could then call Extension but ExtensionEasyBlock does not use super`)
  2. Then PythonPackage.__init__ is called by CargoPythonPackage
  3. super().__init__ in PythonPackage calls Cargo.__init__ again(!)
  4. Then all other methods are called again as per 1.

The MRO order is
[CargoPythonPackage, PythonPackage, Cargo, ExtensionEasyBlock, EasyBlock, Extension] which explains that.

Fix is to consistently use super() in the CargoPythonPackage inheritance chain.

The issue intended to be fixed with easybuilders#2934 is actually caused by Pythons MRO:

CargoPythonPackage explicitely calls `__init__` of **both** `Cargo` and `PythonPackage`
1. `super().__init__` in Cargo calls `ExtensionEasyBlock->EasyBlock` stop (Could then call `Extension` but `ExtensionEasyBlock does not use `super`)
2. Then `PythonPackage.__init__` is called by `CargoPythonPackage`
3. `super().__init__` in PythonPackage calls `Cargo.__init__` again(!)
4. Then all other methods are called again as per 1.

The MRO order is
`[CargoPythonPackage, PythonPackage, Cargo, ExtensionEasyBlock, EasyBlock, Extension]`
which explains that.

Fix is to consistently use `super()` in the CargoPythonPackage inheritance chain.
@Flamefire
Copy link
Contributor Author

Please have a look at this @boegel @Micket (involved in #2934)

Related easyconfig PR: easybuilders/easybuild-easyconfigs#21143

@boegel boegel added the bug fix label Aug 14, 2024
@boegel boegel added this to the release after 4.9.2 milestone Aug 14, 2024
"""Constructor for CargoPythonPackage easyblock."""
Cargo.__init__(self, *args, **kwargs)
PythonPackage.__init__(self, *args, **kwargs)
super().__init__(*args, **kwargs)
Copy link
Member

Choose a reason for hiding this comment

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

This looks very sensible, but I seem to remember there was a reason why we didn't do it like this initially...

@Micket Do you recall why we implemented it like this?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't recall any details here, if there some order that had to be forced or not, or maybe it was just duplicated from configuremakepythonpackage.py.

If it works it works, that's all that matters, so go ahead and fix this.

@Flamefire
Copy link
Contributor Author

As discussed in the confcall I made that compatible with Python 2 which doesn't support argument-less super()

I found another instance of that in Palm and an __init__ which did nothing and could be removed, see https://pylint.readthedocs.io/en/latest/user_guide/messages/warning/useless-parent-delegation.html

@boegel boegel changed the title fix double initialization of Cargo by CargoPythonPackage fix double initialization of Cargo by CargoPythonPackage by removing incorrect custom __init__ implementation + fix use of super() in PALM easyblock (since that doesn't work with Python 2.7) Aug 14, 2024
@boegel
Copy link
Member

boegel commented Aug 14, 2024

@boegelbot please test @ jsc-zen3
EB_ARGS="maturin-1.1.0-GCCcore-12.3.0.eb"

@boegelbot
Copy link

@boegel: Request for testing this PR well received on jsczen3l1.int.jsc-zen3.fz-juelich.de

PR test command 'if [[ develop != 'develop' ]]; then EB_BRANCH=develop ./easybuild_develop.sh 2> /dev/null 1>&2; EB_PREFIX=/home/boegelbot/easybuild/develop source init_env_easybuild_develop.sh; fi; EB_PR=3406 EB_ARGS="maturin-1.1.0-GCCcore-12.3.0.eb" EB_REPO=easybuild-easyblocks EB_BRANCH=develop /opt/software/slurm/bin/sbatch --job-name test_PR_3406 --ntasks=8 ~/boegelbot/eb_from_pr_upload_jsc-zen3.sh' executed!

  • exit code: 0
  • output:
Submitted batch job 4678

Test results coming soon (I hope)...

Details

- notification for comment with ID 2288342661 processed

Message to humans: this is just bookkeeping information for me,
it is of no use to you (unless you think I have a bug, which I don't).

@boegelbot
Copy link

Test report by @boegelbot

Overview of tested easyconfigs (in order)

  • SUCCESS maturin-1.1.0-GCCcore-12.3.0.eb

Build succeeded for 1 out of 1 (1 easyconfigs in total)
jsczen3c1.int.jsc-zen3.fz-juelich.de - Linux Rocky Linux 9.4, x86_64, AMD EPYC-Milan Processor (zen3), Python 3.9.18
See https://gist.github.com/boegelbot/0564556181cc9483d3b637caf21d0ba0 for a full test report.

@boegel boegel merged commit e9ab043 into easybuilders:develop Aug 14, 2024
@Flamefire Flamefire deleted the fix-double-init branch August 15, 2024 12:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants