Skip to content

Remove Python2 style super()-calls and fix ExtensionEasyBlock#3681

Merged
lexming merged 5 commits intoeasybuilders:developfrom
Flamefire:super-calls
May 12, 2025
Merged

Remove Python2 style super()-calls and fix ExtensionEasyBlock#3681
lexming merged 5 commits intoeasybuilders:developfrom
Flamefire:super-calls

Conversation

@Flamefire
Copy link
Copy Markdown
Contributor

@Flamefire Flamefire commented Apr 1, 2025 •
edited
Loading

This removes a lot of "clutter" and avoids C&P mistakes when extending or adding new easyblocks based on existing ones

The test suite then failed due to a bug in our usage:

  • ExtensionEasyBlock overrides make_module_extra from EasyBlock
  • It replaces the altroot/altversion parameters by a single "extra" so when calling it now with the expected arguments, e.g. via super it fails with a TypeError.

Will be fixed and hence requires

@Flamefire Flamefire changed the title Remove Python2 style super()-calls Remove Python2 style super()-calls and fix ExtensionEasyBlock Apr 1, 2025
@Flamefire
Copy link
Copy Markdown
Contributor Author

Flamefire commented Apr 16, 2025

@Micket rebased after merge of the framework PR

@boegel boegel added this to the release after 5.0.0 milestone Apr 23, 2025
@boegel
Copy link
Copy Markdown
Member

boegel commented Apr 23, 2025

@Flamefire So with the framework PR merged, we now have a bug in some easyblocks (the ones that derive from ExtensionEasyBlock, which is being fixed in this PR?

@boegel
Copy link
Copy Markdown
Member

boegel commented Apr 23, 2025

@Flamefire So with the framework PR merged, we now have a bug in some easyblocks (the ones that derive from ExtensionEasyBlock, which is being fixed in this PR?

After checking the framework PR (easybuilders/easybuild-framework#4834) again, it seems like the incorrect usage of just passing extra to make_module_extra is still supported, but deprecated.

At first sight it seems this is only relevant to the RPackage easyblock.
@Flamefire Is that correct, or were there other cases?

@Flamefire
Copy link
Copy Markdown
Contributor Author

Flamefire commented Apr 23, 2025

@Flamefire So with the framework PR merged, we now have a bug in some easyblocks (the ones that derive from ExtensionEasyBlock, which is being fixed in this PR?

It is actually the other way round ExtensionEasyBlock is in framework and some easyblocks make use of the running into the issue where the override change parameters. (There is actually a PyLint warning for this as usually an override should act "similar enough" to the base class [LSP])

At first sight it seems this is only relevant to the RPackage easyblock.

I'm not sure if there are others in e.g. custom easyblocks hence the deprecation only similar to other changes we did. I fixed the RPackage easyblock to avoid triggering our own deprecation warning.

@boegel boegel changed the title Remove Python2 style super()-calls and fix ExtensionEasyBlock Remove Python2 style super()-calls and fix ExtensionEasyBlock Apr 23, 2025
lexming
lexming requested changes Apr 25, 2025
View reviewed changes
Comment thread easybuild/easyblocks/generic/perlbundle.py
Comment thread easybuild/easyblocks/generic/perlbundle.py
Comment thread easybuild/easyblocks/generic/pythonbundle.py
Comment thread easybuild/easyblocks/generic/pythonbundle.py
Comment thread easybuild/easyblocks/generic/pythonbundle.py
Comment thread easybuild/easyblocks/m/mamba.py Outdated
Comment thread easybuild/easyblocks/m/metagenome_atlas.py
Comment thread easybuild/easyblocks/o/openssl_wrapper.py
Comment thread easybuild/easyblocks/t/tkinter.py Outdated
Comment thread test/easyblocks/suite.py
Flamefire
Flamefire commented Apr 28, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor Author

@Flamefire Flamefire left a comment

Choose a reason for hiding this comment

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

@lexming Thanks for the probably exhausting review.

I double checked the instances where there would be a change and reverted them or explained why it makes sense. Please see if you agree.

Comment thread easybuild/easyblocks/m/metagenome_atlas.py
Comment thread easybuild/easyblocks/o/openssl_wrapper.py
Comment thread easybuild/easyblocks/t/tkinter.py Outdated
Comment thread test/easyblocks/suite.py
lexming
lexming reviewed May 5, 2025
View reviewed changes
Comment thread easybuild/easyblocks/i/imkl_fftw.py
@lexming
Copy link
Copy Markdown
Contributor

lexming commented May 5, 2025

Thanks for the update @Flamefire. I agree with the changes/fixes you propose to those super calls that now jump to higher parents, but those changes complicate merging this PR as we should test them. I'll start running those tests and uploading reports.

@lexming
Copy link
Copy Markdown
Contributor

lexming commented May 12, 2025 •
edited
Loading

Test reports for those easyblock with changes beyond replacing super calls to immediate parent with super():

lexming
lexming approved these changes May 12, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@lexming lexming left a comment

Choose a reason for hiding this comment

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

LGTM

@lexming
Copy link
Copy Markdown
Contributor

lexming commented May 12, 2025

Merging, thanks @Flamefire !

@lexming lexming merged commit 3d21594 into easybuilders:develop May 12, 2025
17 checks passed
@Flamefire Flamefire deleted the super-calls branch May 12, 2025 10:20
@branfosj branfosj mentioned this pull request May 12, 2025
Closed
@Crivella Crivella mentioned this pull request May 14, 2025
Merged
@Flamefire Flamefire mentioned this pull request Jun 17, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lexming lexming lexming approved these changes

Assignees

No one assigned

Labels

bug fix code cleanup

Projects

None yet

Milestone

5.1.0

Development

Successfully merging this pull request may close these issues.

3 participants

@Flamefire @boegel @lexming