copy EasyConfig instance in constructor of Bundle and Cargo easyblocks before making changes to it

[boegel]

required to make creating multiple EasyBlock instances from one EasyConfig instance works as intended + fix easybuilders/easybuild-easyconfigs#21393

[boegel]

Test report by @boegel

Overview of tested easyconfigs (in order)

• SUCCESS Xvfb-21.1.9-GCCcore-13.2.0.eb
• SUCCESS matplotlib-3.7.0-gfbf-2022b.eb
• SUCCESS mfqe-0.5.0-GCC-12.3.0.eb
• SUCCESS tokenizers-0.15.2-GCCcore-12.3.0.eb

Build succeeded for 4 out of 4 (4 easyconfigs in total)
node3105.skitty.os - Linux RHEL 8.8, x86_64, Intel(R) Xeon(R) Gold 6140 CPU @ 2.30GHz, Python 3.6.8
See https://gist.github.com/boegel/beb1d58c012af6f896576c87ded12d61 for a full test report.

[Micket]

hm, well, i'm not opposed, but we should make it clear or not if init method are expected to be idempotent or always copy. Seems very inconsistent that

x  = app_class(ec)
print(ec.is_this_data_modified_by_init)

will the input argument ec be modified or not? Now this depends on the easyblock inconsistently. I don't know if there is anywhere in easybuild normal framework that ever looks at the ec file after it has been passed to an app_class, but this would change this behavior. If framework never looks at the input ec parameter, then maybe none of the tests should do that either. If they all always just even avoids naming the ec object at all, i.e.

x = app_class(EasyConfig(file_content))
x.cfg <-- always do this

then tests would always be consistent with normal usage.

[boegel]

I'd say that EasyConfig instances passed to an EasyBlock constructor should never be modified "in place", since that an unwanted side effect that leads to confusion/bugs (see easybuilders/easybuild-easyconfigs#21393).

We're currently not enforcing this, but we probably should.

[Flamefire]

I'd say that EasyConfig instances passed to an EasyBlock constructor should never be modified "in place"

That would mean we should likely do the copy in framework in the EasyBlock class because we have e.g. self.cfg.update and similar in many easyblocks. Not sure if that is always in the ctor or if we expect that the EasyConfig instance is unmodified after some other methods are called on the easyblock.

We could add a check in the easyconfig repo tests, that the EasyConfig instance is unmodified after the ctor because there we instantiate pretty much all easyblocks. We can also add a check in framework at the place where it instantiates the easyblock to fail if the ctor modifies it.

However I'm afraid that doing that check on scale would be expensive as it involves a deep copy and deep comparison. It would slow down the tests and/or runtime quite a bit I guess. But of course we'd need to measure that.

[boegel]

Some fallout of this for easyconfigs test suite is being fixed in:

• take into list of crates for easyconfigs using Cargo-based easyblock when determining checksums for patches easybuild-easyconfigs#21416

[boegel]

I'd say that EasyConfig instances passed to an EasyBlock constructor should never be modified "in place"

That would mean we should likely do the copy in framework in the EasyBlock class because we have e.g. self.cfg.update and similar in many easyblocks. Not sure if that is always in the ctor or if we expect that the EasyConfig instance is unmodified after some other methods are called on the easyblock.

We could add a check in the easyconfig repo tests, that the EasyConfig instance is unmodified after the ctor because there we instantiate pretty much all easyblocks. We can also add a check in framework at the place where it instantiates the easyblock to fail if the ctor modifies it.

However I'm afraid that doing that check on scale would be expensive as it involves a deep copy and deep comparison. It would slow down the tests and/or runtime quite a bit I guess. But of course we'd need to measure that.

Yeah, this likely needs some follow-up.

Always copying the EasyConfig instance makes sense, but there may be significant performance impact though, indeed.