-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Use a module-like object to propagate changes in the MRO, when setting build env #34059
Use a module-like object to propagate changes in the MRO, when setting build env #34059
Conversation
def __getattr__(self, item): | ||
return getattr(self.current_module, item) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this has a version of the same problem that the original PR was trying to fix -- we need to be able to access values from any of the higher modules in the class's MRO.
Could we do something like
def __getattr__(self, item): | |
return getattr(self.current_module, item) | |
module_undefined = object() | |
def __getattr__(self, item): | |
for mod in [self.current_module] + self.modules_in_mro: | |
value = getattr(mod, item, module_undefined) | |
if value != self.module_undefined: | |
return value | |
getattr(self.current_module, item) # raises attribute error with appropriate formatting |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm kind of opposed to this monkey patching also when getting objects. The receiver can't expect any order of execution for the setup_dependent_package
functions, and the objects that might be queried are coming from there, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To give an example, consider one file:
SOME_GLOBAL = 1
class BaseFoo(MakefilePackage):
[ ... ]
another file:
class DerivedFoo(BaseFoo):
[ ... ]
I don't want to allow retrieving:
derived_module.SOME_GLOBAL
since that is explicitly defined elsewhere. I think it's different for global variables that we inject in a controlled manner, like the ones in setup_dependent_package
. In that case we want them to be propagated because a recipe might call super()
and expect some function etc. to be there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By now I'm thinking it would be nicer to just pass a separate object of accumulated keys + values to the build environment, like we already do for environment variables. All this mutable state and global variables is just so ugly, hard to debug, hard to test, and nobody knows the order of evaluation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's the example I was thinking of:
# builtin repo
SOME_GLOBAL = 1
class Foo(Package):
depends_on("bar")
...
# builtin repo
class Bar(Package):
def setup_dependent_package(self, module, dependent_spec):
if hasattr(module, 'SOME_GLOBAL'):
...
# local repo
class Foo(builtin.Foo):
...
In that case the module containing the class Foo
has been effectively turned into part of the interface for Foo
, but the inheriting package would have to reproduce it to be able to interoperate with the dependencies.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@becker33 This is on the packager to fix. They can do:
# builtin repo
class Foo(Package):
SOME_GLOBAL = 1
depends_on("bar")
...
# builtin repo
class Bar(Package):
def setup_dependent_package(self, module, dependent_spec):
if hasattr(dependent_spec.package, 'SOME_GLOBAL'):
...
# local repo
class Foo(builtin.Foo):
...
which holds also for derived classes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean, we should probably be requiring that for everything and getting the module scopes out of the interface. But as long as we allow the builtin packages to have that relationship, we have to support this in the inherited package. The whole point of the inherited package is that you don't have to modify the builtin repo to be able to inherit from it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By now I'm thinking it would be nicer to just pass a separate object of accumulated keys + values to the build environment, like we already do for environment variables. All this mutable state and global variables is just so ugly, hard to debug, hard to test, and nobody knows the order of evaluation.
I'd agree, but since this is part of the package interface we have the drawback of asking users to be more verbose. Instead of:
cmake(...)
they have to do something similar to:
commands["cmake"](...)
I am not sure if that might cause some push back. Also we need to be able to pass this context around to base classes etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But as long as we allow the builtin packages to have that relationship, we have to support this in the inherited package.
Here it's not the inherited *Package
, but the inherited module. We shouldn't be looking into other modules and pretend the variables are defined in this module, except setting downstream what we inject explicitly. We could find all sort of stuff including Builders etc. Also, I didn't try it, but is the current Spack behaving this way?
EDIT: basically I think that the pathological case above should be solved in packages:
# local repo
from spack.pkg.builtin.foo import SOME_GLOBAL
class Foo(builtin.Foo):
...
EDIT 2:
@becker33 What if the user doesn't want to have SOME_GLOBAL
found in the derived package? Should it go and delete it in the base package?
I think my point is that this PR takes care of propagating what Spack injects and somehow we promise will be found in packages. It shouldn't alter module look-up because that will introduce bugs. We should only set things in modules other than the current one that can be reached by super
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should document that any module-scope variables that are part of the package interface (used by a dependency/dependent) should be imported into the inheriting package, but otherwise I'm ok with that as a solution. I think we can expect that level of sophistication from users who are inheriting packages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly LGTM -- one name change requested, but approved because I don't feel strongly about it, so if you feel otherwise you can go ahead and merge.
We should also make sure this is python2 compatible, this would be good to backport for v0.19.1
lib/spack/spack/build_environment.py
Outdated
@@ -1437,3 +1424,51 @@ def write_log_summary(out, log_type, log, last=None): | |||
# If no errors are found but warnings are, display warnings | |||
out.write("\n%s found in %s log:\n" % (plural(nwar, "warning"), log_type)) | |||
out.write(make_log_context(warnings)) | |||
|
|||
|
|||
class ModuleMonkeyPatcher: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think of a name like ModuleChangeAccumulator
instead? I don't think what we're doing is properly monkeypatching here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right it's not proper monkey-patching. I went with ModuleChangePropagator
. I'll take care of Python 2 compatibility when backporting this to the release branch (mainly f-strings and not deriving from object).
…g build env This fixes an issue introduced in spack#32340, which changed the semantics of the "module" object passed to the "setup_dependent_package" callback.
cb6050b
to
30d1b5c
Compare
Can't we use this in the initial build environment setup too btw for props like |
Let's leave that for a follow-up. |
…g build env (spack#34059) This fixes an issue introduced in spack#32340, which changed the semantics of the "module" object passed to the "setup_dependent_package" callback.
This fixes an issue introduced in #32340, which changed the semantics of the "module" object passed to the "setup_dependent_package" callback.