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

Successfully Concretize Python3-Based DAGs #7926

Closed
wants to merge 9 commits into from

Conversation

citibeth
Copy link
Member

This is an on-going PR to collect unmergable changes ("hacks") to Spack packages required to build Python3-based software stacks. The hacks are due to shortcomings in the concretizer, which will be solved someday. Until then, this can serve as a resource. Please add to this PR if you find more changes needed.

@citibeth citibeth added bug Something isn't working concretization python3 labels Apr 26, 2018
@davydden davydden requested a review from balay April 26, 2018 19:22
@@ -114,7 +114,7 @@ class Petsc(Package):
depends_on('mpi', when='+mpi')

# Build dependencies
depends_on('python@2.6:2.8', type='build')
Copy link
Member

@davydden davydden Apr 26, 2018

Choose a reason for hiding this comment

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

can you check if PETSc support 3.x? Maybe that should be

depends_on('python@2.6:2.8,3.abc:', type='build')

Copy link
Member Author

Choose a reason for hiding this comment

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

I've asked the authors. Until then, I'm assuming it's Python2 only.

Copy link
Member

Choose a reason for hiding this comment

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

so the intention is to avoid using Spack's python and hope the user has python 2 in path?

p.s. I thought there were some PRs merged on conretization of build dependencies separately (@adamjstewart @alalazo ?) that might help here to have 2 different version of python in the graph.

p.p.s. you should also adjust slepc then

Copy link
Member Author

Choose a reason for hiding this comment

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

@davydden Yes that is the intention. Whatever concretization fixes were merged, they don't solve this problem. If you try concretizing a software stack that involves python@3 and petsc, concretization will fail. This PR is never going to be merged, it is simply a resource for people to get their Python3 stuff working.

p.p.s. you should also adjust slepc then

Since I don't use slepc, I'm not going to bother.

Copy link
Member

Choose a reason for hiding this comment

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

This PR is never going to be merged, it is simply a resource for people to get their Python3 stuff working.

ah, i see, i missed that.

Copy link
Member

Choose a reason for hiding this comment

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

I thought there were some PRs merged on conretization of build dependencies separately

You're thinking of #2548, which still hasn't been merged ☹️

Copy link
Member

Choose a reason for hiding this comment

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

which still hasn't been merged

☹️

@balay
Copy link
Contributor

balay commented Apr 26, 2018 via email

@citibeth citibeth added the WIP label Apr 26, 2018
@citibeth citibeth changed the title Fixes for Python3 Build Ongoing: Fixes for Python3 Build Apr 26, 2018
@balay
Copy link
Contributor

balay commented Apr 26, 2018 via email

@adamjstewart
Copy link
Member

I'm not a huge fan of PRs that are never intended to be merged...

What about a note in http://spack.readthedocs.io/en/latest/known_issues.html that describes what the problem is and how to resolve it if you run into it?

@citibeth
Copy link
Member Author

citibeth commented Apr 27, 2018 via email

@citibeth
Copy link
Member Author

I realized this could be turned into a mergeable PR. Packages that need it can have a +python3 variant. It would be turned on when concretizing with Python3. I know it's a bit of a crutch compared to fixing the concretizer. But it would work, and be better than what we have now. Interested users would just have to put all: [+python3] in their packages.yaml.

@citibeth citibeth changed the title Ongoing: Fixes for Python3 Build Successfully Concretize Python3-Based DAGs Apr 28, 2018
@citibeth citibeth requested a review from davydden April 28, 2018 21:31
@citibeth
Copy link
Member Author

OK... I've made this PR mergeable. Please see the documentation for how it works. If we can merge this, then we will get a substantial improvement for anyone using Python3 in their software stack.

@citibeth citibeth added ready and removed WIP labels Apr 28, 2018
variant to the problematic package, with the understanding that the
user must set ``all: ['+python3']`` in ``packages.yaml`` for software
stacks involving Python3.

Copy link
Member

Choose a reason for hiding this comment

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

maybe mention #2548 as a WIP which should resolved this issue.

@@ -91,6 +91,8 @@ class Petsc(Package):
multi=False)
variant('suite-sparse', default=False,
description='Activates support for SuiteSparse')
variant('python3', 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 would add a comment above

FIXME: remove once #2548 is merged and tested

Copy link
Member Author

Choose a reason for hiding this comment

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

Or maybe just merge #2548 and forget about this crazy PR. @tgamblin @scheibelp ping???

@svenevs
Copy link
Member

svenevs commented May 2, 2018

Yaaaaay python3!!! 2020 is coming up pretty soon ;) If the decision is to merge this please ping me and I will PR to your PR with other affected packages I've hacked locally for python 3.

I don't know how I feel about introducing the variant, because when it disappears it'll orphan those packages. However, having a PR open even if it doesn't get merged means people can at the very least apply the patch here with two commands (fetch the pr and merge) rather than needing to find them all in their own. That works for me because I only do python3, but may have more serious implications for traditional spack usage (I'm only administering my local system, not a cluster).

@svenevs
Copy link
Member

svenevs commented May 2, 2018

Ugh sorry I read too fast / got too excited. Given the no merge intention, I will submit the PR when I have access to that computer again tonight / tomorrow.

@citibeth
Copy link
Member Author

citibeth commented May 2, 2018

Yaaaaay python3!!! 2020 is coming up pretty soon ;) If the decision is to merge this please ping me and I will PR to your PR with other affected packages I've hacked locally for python 3.

Please create a PR based on this branch (efischer/Python3Fixes), and I will have it added to this PR. If we can collect all the Python3-related fixes in one place, then that will make life easier for others who need it.

Given the no merge intention, I will submit the PR when I have access to that computer again tonight / tomorrow.

At first, this PR was no merge, because it would break Python2. Now with the +python3 variant, it can be merged; hence the "ready" flag.

I don't know how I feel about introducing the variant, because when it disappears it'll orphan those packages.

That's why I left a note on #2548 to revert this PR once it is merged. The more we can collect all our Python3 fixes in one place resulting in a single merge, the easier that reversion will be. At worst, someone (@citibeth?) will have to grep through the packages for python3, determine the commit in which each of those variants was introduced (via git blame), and revert each of those commits. It's really not hard, and Git really is amazing.

That works for me because I only do python3, but may have more serious implications for traditional spack usage (I'm only administering my local system, not a cluster).

@tgamblin Yes that's why we need to merge either this or #2548 sooner, rather than later.

python3: more
@s-sajid-ali
Copy link
Contributor

I've made a separate PR for python-3 only version of ipython. Since ipython and matplotlib along with numpy and scipy are now python-3 only, it's strange to see python-3 support being a variant not the other way around!

@citibeth
Copy link
Member Author

citibeth commented Jun 1, 2018 via email

@s-sajid-ali
Copy link
Contributor

s-sajid-ali commented Jun 8, 2018

I was wondering if we could split the python package into 2 packages, one for python2 and another for python3. This would be a simpler fix than trying to fix the concretizer itself.

Did spack decide against this ?

@citibeth
Copy link
Member Author

citibeth commented Jun 8, 2018 via email

pramodk added a commit to BlueBrain/spack that referenced this pull request Aug 26, 2018
To overcome the limitation of current concretiser in spack.
See spack#7926
pramodk added a commit to BlueBrain/spack that referenced this pull request Aug 26, 2018
To overcome the limitation of current concretiser in spack.
See spack#7926
pramodk added a commit to BlueBrain/spack that referenced this pull request Aug 27, 2018
pramodk added a commit to BlueBrain/spack that referenced this pull request Sep 29, 2018
pramodk added a commit to BlueBrain/spack that referenced this pull request Nov 10, 2018
pramodk added a commit to BlueBrain/spack that referenced this pull request Nov 27, 2018
@citibeth citibeth mentioned this pull request Jan 15, 2019

This can happen for two reasons, both related to the concretizer:

1. A package requires ``python@2`` as a build dependency only (eg:

Choose a reason for hiding this comment

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

The inverse of this happens as well. For instance, mesa required python 3 as a build-only dependency, but paraview requires python 2. Thus paraview+python fails to concretize. The workaround at the moment is to do a two stage install where you first install mesa explicitly. Then when you install par5aview, the concretizer only evaluates the link and runtime dependencies of the installed mesa.

@adamjstewart
Copy link
Member

I've been using:

packages:
    python:
        version: ['3:']

in my packages.yaml for awhile now, and I have yet to run into any Python 3 concretization problems on develop. Is this PR still necessary?

@tgamblin tgamblin deleted the efischer/Python3Fixes branch October 25, 2019 06:48
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

7 participants