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

Make builddependencies iterable. #2741

Merged
merged 33 commits into from
Mar 15, 2019

Conversation

bartoldeman
Copy link
Contributor

This is a list of list of buildependencies. It gets iterated on top
of the common builddependencies. This will allow e.g. to build with
multiple Python versions. A toy test is included.

The "prepare" step is now fully iterated which needed a few adjustments.
Hopefully this doesn't break anything.

This is a list of list of buildependencies. It gets iterated on top
of the common builddependencies. This will allow e.g. to build with
multiple Python versions. A toy test is included.

The "prepare" step is now fully iterated which needed a few adjustments.
Hopefully this doesn't break anything.
Copy link

@houndci-bot houndci-bot left a comment

Choose a reason for hiding this comment

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

Some files could not be reviewed due to errors:

./easybuild/framework/easyconfig/easyconfig.py:201:-470: W605 invalid escape ...
./easybuild/framework/easyconfig/easyconfig.py:201:-470: W605 invalid escape sequence '\ '
./easybuild/framework/easyconfig/easyconfig.py:201:-461: W605 invalid escape sequence '\ '
./easybuild/framework/easyconfig/easyconfig.py:201:-346: W605 invalid escape sequence '\ '
./easybuild/framework/easyconfig/easyconfig.py:201:-269: W605 invalid escape sequence '\ '

Copy link

@houndci-bot houndci-bot left a comment

Choose a reason for hiding this comment

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

Some files could not be reviewed due to errors:

./easybuild/framework/easyconfig/easyconfig.py:201:-470: W605 invalid escape ...
./easybuild/framework/easyconfig/easyconfig.py:201:-470: W605 invalid escape sequence '\ '
./easybuild/framework/easyconfig/easyconfig.py:201:-461: W605 invalid escape sequence '\ '
./easybuild/framework/easyconfig/easyconfig.py:201:-346: W605 invalid escape sequence '\ '
./easybuild/framework/easyconfig/easyconfig.py:201:-269: W605 invalid escape sequence '\ '

Copy link

@houndci-bot houndci-bot left a comment

Choose a reason for hiding this comment

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

Some files could not be reviewed due to errors:

./easybuild/framework/easyconfig/easyconfig.py:201:-470: W605 invalid escape ...
./easybuild/framework/easyconfig/easyconfig.py:201:-470: W605 invalid escape sequence '\ '
./easybuild/framework/easyconfig/easyconfig.py:201:-461: W605 invalid escape sequence '\ '
./easybuild/framework/easyconfig/easyconfig.py:201:-346: W605 invalid escape sequence '\ '
./easybuild/framework/easyconfig/easyconfig.py:201:-269: W605 invalid escape sequence '\ '

easybuild/framework/easyconfig/tweak.py Outdated Show resolved Hide resolved
easybuild/framework/easyconfig/tweak.py Outdated Show resolved Hide resolved
@bartoldeman bartoldeman changed the title Add iterate_builddependencies easyconfig option. Add iterate_builddependencies easyconfig option. (WIP) Jan 31, 2019
@bartoldeman
Copy link
Contributor Author

This works but still needs a bit of cleanup:

  • explore keeping iterate_builddependencies separate at all times, and just iterate over it instead of merging it into the common builddependencies.
  • the resetting of the environment from ready_substeps is tricky. Ideally you'd just unload the module and its deps from the iterated builddependency but that can be tricky. It does state "creating build dir, resetting environment" but I don't quite understand how env.reset_changes() works. @boegel if you can enlighten me when you have time, please let me know.

@boegel boegel added this to the 3.9.0 milestone Jan 31, 2019
@bartoldeman
Copy link
Contributor Author

At the EUM @boegel was asking about why not to extend builddependencies. This is tricky because if you do e.g.

builddependencies =
[[('Python','2.7.15'), ('CMake', '3.12.3')],
  ('Python','3.6.6'), ('CMake', '3.12.3')]]

How do you deal with the common CMake builddependency? Parse it twice? Implement duplicate detection in the framework?

@bartoldeman
Copy link
Contributor Author

I have now implemented proper module unloading. Hopefully it works fine with Tmod.

Actually the way things are now it may not be to hard after all to iterate over builddependencies itself.
There's some duplication but probably little time is spent vs the actual build.

@bartoldeman
Copy link
Contributor Author

Just in case, I've put a patch vs develop for builddependencies without the iterate_builddependencies here keyword here:
https://gist.github.com/d4f03ab150e65237f5cbdf501e93b818

I think it works well but the hiddendependencies code is getting rather cumbersome, so I'll work on a new PR to address that (I suppose we could set dep['hidden']=True instead of needing to remove them and then adding them back in the dump code).

@bartoldeman
Copy link
Contributor Author

#2748 cleans up hiddendependencies.

easybuild/framework/easyblock.py Show resolved Hide resolved
builddeps = self['builddependencies']
if 'builddependencies' in self.iterate_options and not isinstance(builddeps[0], dict):
# flatten if not iterating yet
builddeps = flatten(builddeps)
Copy link
Member

Choose a reason for hiding this comment

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

@bartoldeman During today's conf call you mentioned that one option could be to create two separate methods for this, to clearly discriminate the two situations, and avoid mixing them?

easybuild/framework/easyconfig/easyconfig.py Outdated Show resolved Hide resolved
This allows (build)dependencies() be flattened strictly outside
the iterating process.
@boegel
Copy link
Member

boegel commented Mar 14, 2019

@bartoldeman Problem with failing tests with Python 2.6 should be fixed with #2807, but seems like you broke something else as well.

@boegel
Copy link
Member

boegel commented Mar 15, 2019

(close/re-open to trigger new Travis run now that #2807 is merged)

@boegel boegel closed this Mar 15, 2019
@boegel boegel reopened this Mar 15, 2019
Copy link
Member

@boegel boegel left a comment

Choose a reason for hiding this comment

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

partially fixed with ComputeCanada#15

# take into account that this *dependencies parameter may be iterated over
if key in self.iterate_options:
deps = flatten(deps)
deps_ref = flatten(deps_ref)
Copy link
Member

Choose a reason for hiding this comment

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

@bartoldeman This looks wrong to me... flatten will create a new list, so deps_ref is most definitely no longer a reference to the original list...

Copy link
Member

Choose a reason for hiding this comment

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

Actually, it's OK. deps_ref will be a new list, but the entries inside are still references to the original dependency dict values that we aim to modify, so it's all good...

easybuild/framework/easyconfig/easyconfig.py Outdated Show resolved Hide resolved
easybuild/framework/easyblock.py Outdated Show resolved Hide resolved

if key in parsed_ec.iterate_options:
val = flatten(val)
orig_val = flatten(orig_val)
Copy link
Member

Choose a reason for hiding this comment

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

@bartoldeman Same here, flatten creates a new list, so this is no longer a reference to the original list...

Copy link
Member

Choose a reason for hiding this comment

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

Same here, it's OK to use flatten here, since we're still using references to the original dict values, and that's what wer'e changing (not the list itself).

bartoldeman and others added 2 commits March 15, 2019 11:35
rename 'EasyBlock.reset_changes' to 'EasyBlock.reset_env' + use 'nub'
bartoldeman and others added 2 commits March 15, 2019 12:39
can't use nub to filter out duplicates in flattened list of build dep…
fix filtering of duplicates in builddependencies method
@boegel
Copy link
Member

boegel commented Mar 15, 2019

Tested with modified Eigen easyconfig, which lists two different versions of CMake (there's no point in that, other than testing the support for iterating of a list of lists of build dependencies implemented here):

name = 'Eigen'
version = '3.3.7'

homepage = 'http://%(namelower)s.tuxfamily.org/index.php?title=Main_Page'

description = """Eigen is a C++ template library for linear algebra: matrices, vectors, numerical solvers,
 and related algorithms."""

# only includes header files, so no need for a non-dummy toolchain
toolchain = {'name': 'dummy', 'version': ''}

source_urls = [BITBUCKET_SOURCE]
sources = ['%(version)s.tar.bz2']
checksums = ['9f13cf90dedbe3e52a19f43000d71fdf72e986beb9a5436dddcd61ff9d77a3ce']

# stick to latest CMake 3.9.x, since more recent CMake versions require a C++ compiler that supports C++11,
# which may not be available yet in older OSs (e.g. CentOS 6.x)
builddependencies = [
    [('CMake', '3.9.6')],
    [('CMake', '3.12.1')],
]

moduleclass = 'math'

Works as intended, so I consider this good to go.

Thanks you very much for your hard work on this @bartoldeman, I'm well aware that this was far from trivial...

@boegel boegel merged commit 9dc68dd into easybuilders:develop Mar 15, 2019
# create a list of all options that are actually going to be iterated over
# builddependencies are always a list, need to look deeper down below
self.iterate_options = [opt for opt in ITERATE_OPTIONS
if opt != 'builddependencies' and isinstance(self[opt], (list, tuple))]
Copy link
Member

Choose a reason for hiding this comment

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

@bartoldeman I only realize now that this is done too early...

We're assuming here that once the easyconfig file is parsed, we know whether which easyconfig parameters are to be iterated over. That breaks backwards compatibility in some sense, since before this change we had the ability to tweak the easyconfig parameter values before they were actually being used...

And that's exactly what we're doing with FFTW, where we define the list of configure options to iterate over in self.cfg['configopts'] via run_all_steps...
That value is not picked up for defining self.iterate_options, and hence stuff is broken.

I think we'll need to postpone this to avoid breaking backwards compatibility (yet still handle builddependencies here)...

Copy link
Member

Choose a reason for hiding this comment

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

problem should be fixed with #2810

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.

3 participants