Skip to content
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

Test suite runs repeated __init__ which may incorrectly mutate self.cfg paremeter #21393

Closed
Micket opened this issue Sep 12, 2024 · 4 comments · Fixed by easybuilders/easybuild-easyblocks#3448

Comments

@Micket
Copy link
Contributor

Micket commented Sep 12, 2024

In normal use, the easyblock init methods are only called once, but when running the easybuild-easyconfigs test suite the easyblock may be constructed multiple times, and if the easyconfig cfg is mutated, it may be done so repeatedly.

This also means the problem would not occur running a single specific test.

Most easyblocks don't do much of anything, or just re-initialized the same content each time regardless, making them idempotent, but in the Cargo easyblock, we need to construct the sources list from the given list of crates.
As the sources list isn't necessarily empty from the start (you can have other sources + a list of crates), we can't very well clean out the sources either, and we end up appending the new sources to it.
And when we init multiple times, Cargo.__init__ ends up appending multiple duplicated sources.

After this change
easybuilders/easybuild-easyblocks@a9cadc4#diff-e47f470369ad66d27de6a7cb5fa577eeaf527afa731aa400c0fd20fcbb9c1c45L140
the problem reappeared here:
#21354
where the test suite fails because it's counting 2*x number of sources but only x checksums.

We either have to hack something into Cargo easyblock to make it idempotent,

if not hasattr(self.cfg, 'is_initialized'):
    self.cfg.is_initialized = True

or something like clearing out the list of crates again.

@boegel boegel added this to the release after 4.9.3 milestone Sep 14, 2024
@boegel
Copy link
Member

boegel commented Sep 14, 2024

#21392 is blocked due to the same problem.

@Flamefire mentioned he would look into it.

@boegel
Copy link
Member

boegel commented Sep 15, 2024

This is also causing trouble in:

test_pr_sha256_checksums is failing with "List of sources for bundle itself must be empty" for jax-0.4.25-gfbf-2023a.eb, which doesn't make sense because there's no top-level sources set in jax-0.4.25-gfbf-2023a.eb...

ERROR: test_pr_sha256_checksums (test.easyconfigs.easyconfigs.EasyConfigTest)
Make sure changed easyconfigs have SHA256 checksums in place.
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/runner/work/easybuild-easyconfigs/easybuild-easyconfigs/test/easyconfigs/easyconfigs.py", line 1113, in test_pr_sha256_checksums
    checksum_issues = check_sha256_checksums(ecs, whitelist=whitelist)
  File "/opt/hostedtoolcache/Python/3.6.15/x64/lib/python3.6/site-packages/easybuild/framework/easyconfig/tools.py", line 712, in check_sha256_checksums
    eb = eb_class(ec)
  File "/opt/hostedtoolcache/Python/3.6.15/x64/lib/python3.6/site-packages/easybuild/easyblocks/generic/pythonbundle.py", line 59, in __init__
    super(PythonBundle, self).__init__(*args, **kwargs)
  File "/opt/hostedtoolcache/Python/3.6.15/x64/lib/python3.6/site-packages/easybuild/easyblocks/generic/bundle.py", line 83, in __init__
    raise EasyBuildError("List of sources for bundle itself must be empty, found %s", self.cfg['sources'])
easybuild.tools.build_log.EasyBuildError: "List of sources for bundle itself must be empty, found [{'source_urls': ['https://github.com/google/jax/archive/'], 'filename': 'jaxlib-v0.4.25.tar.gz'}, {'source_urls': ['https://github.com/openxla/xla/archive'], 'download_filename': '4ccfe33c71665ddcbca5b127fefe8baa3ed632d4.tar.gz', 'filename': 'xla-4ccfe33c.tar.gz', 'extract_cmd': 'mkdir -p /dummy/builddir/archives && cp %s /dummy/builddir/archives'}, {'source_urls': ['https://github.com/tensorflow/runtime/archive'], 'download_filename': '0aeefb1660d7e37964b2bb71b1f518096bda9a25.tar.gz', 'filename': 'tf_runtime-0aeefb16.tar.gz', 'extract_cmd': 'mkdir -p /dummy/builddir/archives && cp %s /dummy/builddir/archives'}]"

@boegel
Copy link
Member

boegel commented Sep 15, 2024

The problem with the modifications done in #21143 is that the .copy() being done is done too late: when an EasyConfig instance is created for an easyconfig that uses a Bundle easyblock, the top-level sources easyconfig parameter is being modified when creating the instance, see https://github.com/easybuilders/easybuild-easyblocks/blob/easybuild-easyblocks-v4.9.3/easybuild/easyblocks/generic/bundle.py#L169

When the copy is taken, it simply keeps the populated sources value, so it's "too late" in some sense.

@Flamefire
Copy link
Contributor

The problem with the modifications done in #21143 is that the .copy() being done is done too late: when an EasyConfig instance is created for an easyconfig that uses a Bundle easyblock, the top-level sources easyconfig parameter is being modified when creating the instance, see https://github.com/easybuilders/easybuild-easyblocks/blob/easybuild-easyblocks-v4.9.3/easybuild/easyblocks/generic/bundle.py#L169

When the copy is taken, it simply keeps the populated sources value, so it's "too late" in some sense.

Ok so that change just fixed the opposite case: We ended up with NO sources, now we have them twice.

With the change in easybuilders/easybuild-easyblocks#3448 we should remove the copy at https://github.com/easybuilders/easybuild-easyconfigs/pull/21143/files#diff-c85885b053bd84deb5003a873b182bfc52bd3d72a3f3ab3b0bb23f18215f7c70R1051-R1059 because that should work now, shouldn't it? If not we have another similar bug

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment