Skip to content

Conversation

@boegel
Copy link
Member

@boegel boegel commented Mar 18, 2019

Follow-up for #2741, which adds some syntactic sugar to make it a bit more user friendly...

Without this, you would have to use a construct like to following to iterative over multiple Python versions:

common_builddeps = [('CMake', '3.12.1')]
builddependencies = [
    common_builddeps + [('Python', '2.7.15')],
    common_builddeps + [('Python', '3.7.2')],
]

This becomes a lot cleaner using the multi_deps support being added here:

builddependencies = [('CMake', '3.12.1')]
multi_deps = {'Python': ['2.7.15', '3.7.2']}

edit: This PR has grown a bit beyond simply adding support for the multi_deps easyconfig parameter, it also includes a bunch of other (fairly minor) changes that are required to make installing of Python packages for multiple Python versions in a single installation prefix work correctly (see also easybuilders/easybuild-easyblocks#1664), in particular w.r.t. the sanity check.

To make reviewing this PR a bit easier, here's a summary of the changes:

  • Restoring things after iterating is now done via a new post_iter_step, whereas in Make builddependencies iterable. #2741 it was done via restore_iterate_opts which was called during cleanup_step.
    This change was needed to ensure that restoring after iterating was done before the sanity check step, since we need partially iterate again during the sanity check in case multi_deps is used.

  • The extensions_step was moved into the list of iterative steps, to support installing bundles of Python packages (via PythonBundle) for multiple Python versions.
    In addition, the easyconfig parameters for extensions are also restored now in post_iter_step.

  • A new (private) method _sanity_check_step_multi_deps has been implemented, which gets called for installations where multi_deps was used.
    It performs the sanity check iteratively, once for each set of dependencies specified via multi_deps, by passing a list of extra_modules that should be loaded before doing the sanity check.
    Also for the extension sanity check part of ExtensionEasyBlock.sanity_check_step is now done iteratively if multi_deps is used, by loading those dependencies as additional modules.
    These changes are required because those dependencies (for example, Python) are typically required to make sanity check commands pass, or to correctly check for sanity check paths that include templates like %(pyshortver)s.

  • builddependencies are now also taken into account to obtain values for templates, but only while iterating... This is required to ensure correctly resolving templates like %(pyshortver)s when multi_deps is used for Python, for example.

@boegel boegel added this to the next release (3.8.2) milestone Mar 18, 2019
@boegel boegel requested a review from bartoldeman March 18, 2019 19:35
@akesandgren
Copy link
Contributor

akesandgren commented Mar 18, 2019

Does this handle stuff with versionsuffix too?
Like building with this
('GObject-Introspection', '1.54.1', '-Python-2.7.15'), and
('GObject-Introspection', '1.54.1', '-Python-3.6.6'),

Probably a sloppy example, but you get the point.

@akesandgren
Copy link
Contributor

And this is only for builddeps so far?

@boegel
Copy link
Member Author

boegel commented Mar 18, 2019

@akesandgren With this in place, we wouldn't need versionsuffixes anymore to discriminate modules with different Python versions (not sure if that's going to work out though...).

I need to take a closer look at how this will work together with templates like %(pyver)s, especially in the context of the sanity check (which is not part of the list of steps that are being iterated over).

multi_deps leverages the iterative builddependencies support, because that's the only way: you can't have a runtime dependency on multiple different versions of the same software at once...

@akesandgren
Copy link
Contributor

Well, python as version suffix might have been a bad choice. There might be other things that could be in builddeps with varying versionsuffix, but i don't have an explicit example. Just throwing my thoughts into the wild...

@boegel
Copy link
Member Author

boegel commented Mar 19, 2019

@akesandgren Ah, I think I misunderstood the question...

For now, it only supports name to version mappings in multi_deps.

If the needed arises to have versionsuffix support too, we can add that later, by also allowing tuple values in the lists in multi_deps?

'builddependencies': [[], "List of build dependencies", DEPENDENCIES],
'dependencies': [[], "List of dependencies", DEPENDENCIES],
'hiddendependencies': [[], "List of dependencies available as hidden modules", DEPENDENCIES],
'multi_deps': [{}, "Dict of lists of dependency versions over which to iterate", DEPENDENCIES],
Copy link
Member

Choose a reason for hiding this comment

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

I know that this only makes for the case of build_deps but I still think that there's no harm in making that connection explicit here (for those who haven't thought so deeply about it), how about multi_builddeps?

Copy link
Member Author

@boegel boegel Mar 20, 2019

Choose a reason for hiding this comment

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

I guess that could make sense, but it's kind of double...

What we're doing is installing something for multiple versions of a particular dependency (e.g. Python) in a single installation prefix.
Those dependencies are generally runtime dependencies, but during the installation process we only use them as build dependencies, to avoid that they are included as a runtime dep in the generated module file.
That's the only way we can iterate over dependencies without introducing conflicts in the generated module file.
That this is done by injecting stuff into builddependencies doesn't have to be explicit imho, it's kind of an implementation detail...

One concern with the multi-Python installations (raised by @Micket) is that users will have to know to load a Python module as well for things to work.
This could be remedied by loading one of the Python versions (e.g. the first listed in multi_deps) by default, if no other Python version is loaded already (perhaps with an accompanying message). The logical choice there would be the Python 3.x version. Note that this still leaves the door open to use another Python version, which is just a module swap away...

I haven't made up my mind how to implement that yet (and it should probably be part of this PR)...
It could be done in general in the framework, or only for Python packages (but then we'll need to touch multiple generic easyblock like PythonPackage, PythonBundle, etc.).

Copy link
Member

Choose a reason for hiding this comment

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

Ok, that makes sense.

Regarding letting users know a Python module is required is something that can be handled with depends_on('Python') (for Lmod) without providing a version. That way the default is chosen by the site, without them doing something explicit it would be the latest version of Python 3. This could also work in general, depends_on('%(name)s'). Of course it would mean that if they had multiple subversions of Python the dep tree might change.

Copy link
Member Author

Choose a reason for hiding this comment

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

@ocaisa That won't work imho, since you'd be in trouble when a new Python 3.x module is installed with a different subtoolchain...

Copy link
Member

Choose a reason for hiding this comment

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

There you go with your flat naming scheme ruining the party again!

Copy link
Member Author

Choose a reason for hiding this comment

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

Support for loading a default module is not implemented yet in this PR, I'll do that in a follow-up PR to avoid making this PR too big/complicated...

It's a nice enhancement to have, but not a strict requirement to make this work...

Copy link
Member

Choose a reason for hiding this comment

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

+1 to the depends_on support in the future. One could have a guard, to check if the MNS is hierarchical or not, and use depends_on selectively on those cases. But the guard might not be trivial to make it general for custom MNS

@akesandgren
Copy link
Contributor

Yeah if adding tuple support is easy later on then that's good enough.

@boegel boegel changed the title add support for multi_deps easyconfig paramter (syntactic sugar for iterative list of builddependencies) add support for multi_deps easyconfig parameter (syntactic sugar for iterative list of builddependencies) Mar 22, 2019
@boegel boegel changed the title add support for multi_deps easyconfig parameter (syntactic sugar for iterative list of builddependencies) [[WIP]] add support for multi_deps easyconfig parameter (syntactic sugar for iterative list of builddependencies) Mar 23, 2019
@boegel boegel changed the title [[WIP]] add support for multi_deps easyconfig parameter (syntactic sugar for iterative list of builddependencies) add support for multi_deps easyconfig parameter (syntactic sugar for iterative list of builddependencies) Mar 24, 2019
@easybuilders easybuilders deleted a comment from boegelbot Mar 25, 2019
damianam
damianam previously approved these changes Mar 26, 2019
Copy link
Member

@damianam damianam left a comment

Choose a reason for hiding this comment

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

lgtm. Is there something else you'd like to add @boegel ? Any further comment before I merge it @bartoldeman ?

@boegel
Copy link
Member Author

boegel commented Mar 26, 2019

@damianam I've retested this with easybuilders/easybuild-easyconfigs#7921, I think it's good to go now, I don't see any further problems with it...

@bartoldeman
Copy link
Contributor

Consider this approved by me once the easyconfig dump has been fixed (see my PR above).

Copy link
Contributor

@bartoldeman bartoldeman left a comment

Choose a reason for hiding this comment

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

see boegel#43 for the dump changes.

I was thinking about disallowing iterated builddependencies from the user perspective but indeed it doesn't make a lot of sense, as multi_deps are taken into account before the dependencies are parsed and a combination of both is already not allowed.

@boegel boegel dismissed bartoldeman’s stale review March 28, 2019 10:10

PR merged to fix issue with dumped easyconfig

Copy link
Member

@damianam damianam left a comment

Choose a reason for hiding this comment

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

Took a bit, but lgtm

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.

5 participants