Skip to content

Ignore other classes if software specific easyblock class was found #4769

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.

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

Merged
merged 11 commits into from
Mar 3, 2025

Conversation

Flamefire
Copy link
Contributor

If we have other classes next to a software specific easyblock class in an easyblock file they are likely supplemental.

So ignore them in avail_easyblocks.

Also 2 cleanups:

  • Use the constant for EB_ in all places
  • Make the loop a bit simpler by reducing the 8-level nesting

If we have other classes next to a software specific easyblock class
in an easyblock file they are likely supplemental.
So ignore them in `avail_easyblocks`.
- Reduce indentation
- Assign directly to dict
Comment on lines 789 to 794
if len(class_names) > 1:
# If there is exactly one software specific easyblock we use that
sw_specific_class_names = [name for name in class_names
if not is_generic_easyblock(name)]
if len(sw_specific_class_names) == 1:
class_names = sw_specific_class_names
Copy link
Contributor

Choose a reason for hiding this comment

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

This could possibly be improved to allow additional classes in a generic easyblock by checking if pkg is easybuild.easyblocks.generic and comparing the class_names with the filename

Copy link
Contributor Author

@Flamefire Flamefire Feb 24, 2025

Choose a reason for hiding this comment

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

Good point. Implemented. By testing I also found that our (for some reason multi-line regex) would match:

class Foo:
    pass

class Bar(EasyBlock):
   pass

Resulting in the "class_names" Foo:\n pass\n\n\nclass Bar because it finishes only at parentheses not the colon

That was fine for Python2 where the parentheses were mandatory in all cases

The class name regex is MULTILINE and hence might start a match at a
class name without a super-class and match until another class name with a super-class

Restrict by excluding colons in addition to parentheses

Also add test for this and the multi-class case to a generic easyblock
Unrelated class in an easyblock file have the same package name as the
easyblock and hence the filter doesn't remove them.
Instead of the package name check if the class derives from `EasyBlock`
which is a much more reliable check whether the class is an EasyBlock.
@Flamefire
Copy link
Contributor Author

Flamefire commented Feb 24, 2025

Through the latest error I've found a duplication of the logic: avail_easyblocks and get_easyblock_classes do mostly the same while the former could use the latter.

get_easyblock_classes imports all matching modules and gets all classes that derive from EasyBlock (after my fix which might change behavior if one does not. Previously it returned any class defined in the file but I don't think this is an issue. I added a check if any was found, the case of multiple being found might also need handling)

avail_easyblocks also imports the module and then checks the paths defined by the module, then goes over each file and uses a regexp on the text content.

I'd say we should only use the logic from get_easyblock_classes and add the check for no or multiple found (potential) easyblocks there.
Not fully sure how duplication is handled when --include-easyblocks* is in effect though.

@Flamefire Flamefire force-pushed the multi-class-easyblocks branch from 992e175 to 94b9a72 Compare February 24, 2025 09:59
@Flamefire
Copy link
Contributor Author

@akesandgren Shall we fix the duplication issue here, in a separate PR or not at all? I'd say in a follow-up

@akesandgren
Copy link
Contributor

akesandgren commented Feb 24, 2025

Through the latest error I've found a duplication of the logic: get_easyblock_classes and get_easyblock_classes do mostly the same while the former could use the latter.

I assume you meant get_easyblock_classes and avail_easyblocks here?

If so let's push that to a separate PR

@Flamefire
Copy link
Contributor Author

I assume you meant get_easyblock_classes and avail_easyblocks here?

Indeed. It's seemingly C&P-mistake-monday ;-) Fixed so the description makes sense

@akesandgren
Copy link
Contributor

This now looks good to me
@boegel any thoughts on this?

@akesandgren akesandgren requested a review from boegel February 24, 2025 15:02
@akesandgren akesandgren dismissed their stale review February 24, 2025 15:03

Requested changed handled

@boegel boegel added this to the release after 4.9.4 milestone Mar 3, 2025
Copy link
Member

@boegel boegel left a comment

Choose a reason for hiding this comment

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

lgtm

@boegel boegel enabled auto-merge March 3, 2025 19:43
@boegel boegel merged commit 6e68abe into easybuilders:develop Mar 3, 2025
37 checks passed
@Flamefire
Copy link
Contributor Author

Through the latest error I've found a duplication of the logic: avail_easyblocks and get_easyblock_classes do mostly the same while the former could use the latter.

If so let's push that to a separate PR

@boegel Shall I do that PR?

@Flamefire Flamefire deleted the multi-class-easyblocks branch March 4, 2025 08:03
@boegel boegel modified the milestones: release after 4.9.4, release after 5.0.0, 5.0.0 Mar 17, 2025
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