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

adios2 package configuration cleanup #13107

Merged
merged 1 commit into from Oct 14, 2019
Merged

Conversation

williamfgc
Copy link
Contributor

Provide up to date specs for build options
Remove non-production versions
Remove Python 2 support
Format using autopep8

@ax3l @chuckatkins @olesenm please review

@williamfgc williamfgc force-pushed the adios2_cleanup branch 2 times, most recently from b450a0b to da0f2ce Compare October 9, 2019 13:26
@adamjstewart
Copy link
Member

We don't normally remove older releases from packages so that people can reproduce their installations. Also, is it possible to keep Python 2 support, at least for older releases? A lot of users are still stuck with Python 2.

@williamfgc
Copy link
Contributor Author

williamfgc commented Oct 9, 2019

@adamjstewart thanks for your input. The removed versions were alpha versions which are not currently maintained or used. I can put them back if that's the preference, but that would add unnecessary burden to our maintenance tasks.

I might need your help on Python 2 support, I tried running on Ubuntu 18 and got the following error similar to #11531

while trying to concretize the partial spec:

    py-numpy@1.17.2%gcc@7.4.0+blas+lapack arch=linux-ubuntu18.04-sandybridge


py-numpy requires python version 3.5:, but spec asked for 2.7.16

with settings:

extends('python', when='+python')
    depends_on('python@2.7:', type=('build', 'run'), when='+python')
    depends_on('py-numpy@1.6.1:', type=('build', 'run'), when='+python')
    depends_on('py-mpi4py@2.0.0:', type=('build', 'run'), when='+mpi +python')

Please let me know if I am doing something wrong here. Is there a good practice for supporting Python 2 and 3 at the same time? Moving to Python 3.5 works just fine. I'd happy to continue Python 2 support if there is a standard way that doesn't add to our maintenance costs as most of our customers/systems are pulling the plug on Python 2.

@adamjstewart
Copy link
Member

py-numpy requires python version 3.5:, but spec asked for 2.7.16

This is a known bug in the concretizer. Basically, Spack sees that the default Python version is Python 2, but later finds out that NumPy requires Python 3, so crashes. The concretizer should be smart enough to figure this out. @tgamblin is currently working on this. In the meantime, users can either set their default version of Python to be Python 3, or can explicitly request it on the command line.

Is there a good practice for supporting Python 2 and 3 at the same time? Moving to Python 3.5 works just fine. I'd happy to continue Python 2 support if there is a standard way that doesn't add to our maintenance costs as most of our customers/systems are pulling the plug on Python 2.

Do you mean a good practice within your library, or within the Spack package? Within your library, I would use the six module. Within the Spack package, you can do something like:

depends_on('python@2.7:2.8', when='@:1.2.3', type=('build', 'run'))
depends_on('python@3.4:', when='@1.2.4:', type=('build', 'run'))

if you want to drop Python 2 support but keep older versions available.

@williamfgc
Copy link
Contributor Author

williamfgc commented Oct 9, 2019

@adamjstewart thanks for your reply and explanation. Sorry for the confusion, yes, I meant seamless Python 2 and 3 on spack. In general, adios2 looks for Python 3 (and dependencies) first, then Python 2, in that order. Our plan is to drop Python 2 (sooner than later) and only work with Python 3 (preferably since adios2 v2.5.0 introduce python scripts), but that doesn't mean older adios2 versions can't work with Python 3 (in fact, they all do). Based on your suggestion, we have to pick Python 2 or 3 for a specific adios2 version, am I understanding this correctly? Nonetheless, if this is a known bug on spack, I'd be happy to add seamless Python 2 and 3 support for any adios2 version in spack once it's fixed.

When it comes to this PR, I'd rather keep a clean slate to provide long-term support, the current file has been unmaintained for a while and the priority is to prevent that situation in the future.

@adamjstewart
Copy link
Member

Based on your suggestion, we have to pick Python 2 or 3 for a specific adios2 version, am I understanding this correctly?

Nope, that was just a simple example. You can definitely do something like:

depends_on('python@2.7:2.8,3.4:', when='@:1.2.3', type=('build', 'run'))
depends_on('python@3.4:', when='@1.2.4:', type=('build', 'run'))

In this case, versions 1.2.3 and below can work with Python 2.7 or 3.4+, while versions 1.2.4+ can only work with Python 3.4+.

@williamfgc
Copy link
Contributor Author

@adamjstewart OK, thanks! I think I got a good compromise based on your solution. I added Python 2 support for versions before v2.4.0 so the (Python 3) user will have to pass spack install adios2 ^python@3.5 , but only spack install adios2 for the latest version. Thanks a lot, please keep up with the excellent work spack is doing.

Provide up to date specs for build options
Remove non-production versions
Format using autopep8 and flake8
Separate cmake options in adios2 2.4.0 version
Keep default as True as much as possible
Support for Python 2 and 3 for adios2 versions <= 2.4.0
Starting v2.5.0 only Python 3 is supported
Addressing review suggestions
@williamfgc
Copy link
Contributor Author

Pushing again to unclog Travis

@ax3l ax3l requested review from ax3l and chuckatkins October 9, 2019 19:22
variant('shared', default=True,
description='Also build shared libraries')
variant('pic', default=True,
description='Enable position independent code '
'(for usage of static in shared downstream deps)')
variant('mpi', default=True,
description='Enable MPI')
variant('endian_reverse', default=False,
Copy link
Member

Choose a reason for hiding this comment

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

I am not sure if we can do that here or in a packages.yaml, but changing the default depending on arch to default=True for big endian machines would be cool. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ax3l thanks for your suggestion. This flag is for the very special case in which data is generated with one endianness and read on a system with a different endianness. As long as the endianness is the same between writers and readers keeping it False works fine. At the moment this was done for one user, so makes sense that the default is False. It's also very difficult to get the resources to test new cases (I don't have access to a big endian machine at the moment, nor that I will have in the near future). We assume those mixing big/little endian systems in 2019 know the risks of not being the norm.

Copy link
Member

@ax3l ax3l Oct 14, 2019

Choose a reason for hiding this comment

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

That confuses me. Shouldn't the .bp file format detect such moved data sets?
I had this situation in the past quite naturally at blue gene q + support systems, feeding of the same filesystem.

Even if .bp does not encode this (:scream: ?), it should be a runtime option because the data source that is read-in might be on- or off-system in a workflow.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ax3l yes, the bp format always encodes the endianness, this option is for the reader (not the writer) to reverse the endianness if necessary. Keeping the default as OFF is about the probability of mixing endianness + lack of testing from our side.

Copy link
Member

Choose a reason for hiding this comment

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

Migrated off-topic discussion to ornladios/ADIOS2#1822

@ax3l ax3l merged commit dafee9d into spack:develop Oct 14, 2019
jrmadsen pushed a commit to jrmadsen/spack that referenced this pull request Oct 30, 2019
Provide up to date specs for build options
Remove non-production versions
Format using autopep8 and flake8
Separate cmake options in adios2 2.4.0 version
Keep default as True as much as possible
Support for Python 2 and 3 for adios2 versions <= 2.4.0
Starting v2.5.0 only Python 3 is supported
Addressing review suggestions
@williamfgc williamfgc deleted the adios2_cleanup branch January 12, 2021 11:51
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.

None yet

4 participants