easybuilders / easybuild-easyblocks

Collection of easyblocks that implement support for building and installing software with EasyBuild.
https://easybuild.io
GNU General Public License v2.0
106 stars 285 forks source link

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

Closed boegel closed 2 months ago

boegel commented 2 months ago

required to make creating multiple EasyBlock instances from one EasyConfig instance works as intended + fix https://github.com/easybuilders/easybuild-easyconfigs/issues/21393

boegel commented 2 months ago

Test report by @boegel

Overview of tested easyconfigs (in order)

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 commented 2 months ago

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 commented 2 months ago

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 https://github.com/easybuilders/easybuild-easyconfigs/issues/21393).

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

Flamefire commented 2 months ago

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 commented 2 months ago

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

boegel commented 2 months ago

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.