Skip to content

Conversation

@Flamefire
Copy link
Contributor

@Flamefire Flamefire commented Apr 1, 2025

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
Contributor Author

@Micket rebased after merge of the framework PR

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

@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
Copy link
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.

@lexming
Copy link
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
Contributor

lexming commented May 12, 2025

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

Copy link
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
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
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.

3 participants