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.

Sign up for GitHub

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

Jump to bottom

No more symlinks or setting PYTHONPATH #199

Closed
wants to merge 1 commit into from
Closed

No more symlinks or setting PYTHONPATH #199

wants to merge 1 commit into from

Conversation

ketch
Copy link
Member

@ketch ketch commented Nov 21, 2018

This updates our installation instructions to reflect clawpack/clawpack#141 and clawpack/clawpack#138. Specifically, there is no longer an option to just create symlinks (since our installation process doesn't create symlinks anymore anyway) and we no longer instruct users to set PYTHONPATH.

I also added the --user option everywhere that we suggest using pip install.

@mjberger
Copy link
Collaborator

mjberger commented Nov 21, 2018 via email

@ketch
Copy link
Member Author

ketch commented Nov 21, 2018

@mjberger Can you tell me exactly how you install? Specifically, do you use symlink-only? That option is actually not going to be available anymore.

@mjberger
Copy link
Collaborator

mjberger commented Nov 21, 2018 via email

@ketch
Copy link
Member Author

ketch commented Nov 21, 2018

@mjberger It's probably better to have this conversation here, since there's already a lot of relevant information there:

clawpack/clawpack#138

@ketch
Copy link
Member Author

ketch commented Nov 21, 2018

@mjberger Also, I believe that clawpack/clawpack#141 will eliminate the kind of problem you're describing.

We are the only Python package in existence (as far as I know) that recommends setting your PYTHONPATH manually on installation.

@mjberger
Copy link
Collaborator

mjberger commented Nov 21, 2018 via email

@ketch
Copy link
Member Author

ketch commented Nov 21, 2018

No.

@rjleveque
Copy link
Member

I discovered that even though the symlinks are not set, it is still possible to set PYTHONPATH to point to the top level of such an installation and this will over-ride whatever is hidden in the easy-install.pth created by pip (i.e. things like from clawpack import pyclaw will still work). [But this might be system dependent??]

At any rate this isn't recommended I know, but maybe useful for those of us who still want to use PYTHONPATH to switch versions easily. Instead of using pip install -e . I think you can just do python setup.py git-dev to set up an installation to work this way, to avoid pip entirely.

I think.

But maybe it's best not to try to document this backdoor way on the main docs.

@ketch
Copy link
Member Author

ketch commented Nov 22, 2018

it is still possible to set PYTHONPATH to point to the top level of such an installation and this will over-ride whatever is hidden in the easy-install.pth created by pip (i.e. things like from clawpack import pyclaw will still work)

Did you try actually using this installation? I think that your pyclaw is an empty namespace and nothing will work. And now what you've done will hide your actual working installation created by pip.
This is another good reason not to set your PYTHONPATH.

To actually make this approach work, all you would need to do is move all the Python code up to the clawpack/x/ directories. Then @rjleveque and @mjberger wouldn't need to run pip or setup.py at all. I would be 100% in favor of this change.

@rjleveque
Copy link
Member

@ketch: Well, things worked fine in amrclaw and geoclaw (including the Python aspects), which is what us PYTHONPATH addicts care about, but not for running pyclaw examples. However, I'm not sure moving python code up to clawpack/x would help, maybe you can tell from the error below.

Moving the code up doesn't seem necessary for amrclaw or geoclaw, and wouldn't that clutter up the top directories way too much if I understand your suggestion?

When I tried:

from clawpack.pyclaw import examples
claw = examples.shock_bubble_interaction.setup()

it started successfully compiling many things with f2py, but eventually threw an error:

...
/Users/rjl/D/clawpack_test_install/pyclaw/src/pyclaw/util.py:84: UserWarning: successfully executed python setup.py build_ext -i in /Users/rjl/D/clawpack_test_install/pyclaw/examples/shallow_sphere
  warnings.warn("successfully executed python setup.py build_ext -i in %s" % working_dir)

---------------------------------------------------------------------------
AttributeError                            Traceback (most recent call last)
<ipython-input-2-4dec5489d10f> in <module>()
      1 from clawpack.pyclaw import examples
----> 2 claw = examples.shock_bubble_interaction.setup()

/Users/rjl/D/clawpack_test_install/pyclaw/examples/euler_2d/shock_bubble_interaction.py in setup(use_petsc, solver_type, outdir, kernel_language, disable_output, mx, my, tfinal, num_output_times)
    190         solver.lim_type   = 2
    191     else:
--> 192         solver = pyclaw.ClawSolver2D(riemann.euler_5wave_2D)
    193         solver.step_source = step_Euler_radial
    194         solver.source_split = 1

AttributeError: 'module' object has no attribute 'euler_5wave_2D'

@rjleveque
Copy link
Member

Some comments/questions on the new docs:

  1. The section "Install from a tar file" still says to run python setup.py install but should this now be pip install --user -e . ?

  2. Or should we eliminate tar files altogether and only support downloading via pip or git?

  3. In the same section, there is still a reference to install_no-pyclaw in installing.rst although this section no longer exists. Do we have advice for what to do if pyclaw doesn't compile and the user doesn't mind, since she is only planning to use geoclaw, for example?

  4. What if compilation errors arise when using pip install? Is there a good way to recover and get the path set properly at least for use in amrclaw or geoclaw?

  5. In installing_pip.rst there's a section "Notes on using pip to install" that suggests how to install a different tagged version. This should probably be consolidated with instructions on how to switch to a different local version as well? (By doing pip install --user -e . in the relevant top level directory.) This will at least be important for developers. Also perhaps for users, particularly if they have installed multiple versions from tar files?

@ketch
Copy link
Member Author

ketch commented Nov 24, 2018

I thought about this quite a bit after our Skype call last night. I think we're at an impasse:

  1. The maintainers of AMRClaw and GeoClaw insist that these packages be "installable" without using pip.

  2. The person responsible for packaging PyClaw and Riemann (me) insists that these packages' installation make use of pip (or a binary install-based package manager like Conda).

I think we're past debating whether one or the other of these positions is more reasonable. The current status quo seems to be an acceptable (but not very happy) one for maintainers of AMRClaw and GeoClaw, but accommodating both #1 and #2 cost me several hours when releasing 5.5.0, and I can't guarantee that I'll be able to do this in the future.

I would like to know if there are objections to distributing PyClaw/Riemann as a separate package from AMRClaw/Geoclaw.

@mandli
Copy link
Member

mandli commented Nov 26, 2018

Given that AMRClaw and GeoClaw still depend on PyClaw/Riemann I am not sure that this is exactly tenable. I for one would like to maintain a pip installation and have that be the suggested way to install for all users. Otherwise I would also be in favor of making AMRClaw/GeoClaw be more of a library.

@rjleveque
Copy link
Member

@ketch - I thought we decided that the newest pip approach might work fine, but I wanted to test it some more with @mjberger next week before merging. So don't give up on us yet.

When we talked the other day I said that when I download the latest version via

git clone https://github.com/clawpack/clawpack.git
cd clawpack
git checkout cleaner_install
git submodule init
git submodule update

but then do not do the pip install, but instead set PYTHONPATH to the top level directory, I could still import things from clawpack.amrclaw or clawpack.geoclaw just fine, even though there are no symbolic links and it's not clear how Python knows where to find these things. You hypothesized that since I had once used pip to install an earlier version, this information must be stored in some way (if I recall correctly).

So to test this I just created a brand new AWS EC2 instance and did the above (with no pip install). Then when I set PYTHONPATH it works just fine there too. I'll email the full log of what I did, but here's what I mean...

[ec2-user@ip-172-30-0-182 ~]$ ls /home/ec2-user/clawpack/clawpack
__init__.py  __init__.pyc  setup.py

[ec2-user@ip-172-30-0-182 ~]$ export PYTHONPATH=/home/ec2-user/clawpack

[ec2-user@ip-172-30-0-182 ~]$ python
Python 2.7.14 (default, Jul 26 2018, 19:59:38) 
[GCC 7.3.1 20180303 (Red Hat 7.3.1-5)] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> import clawpack
>>> clawpack
<module 'clawpack' from '/home/ec2-user/clawpack/clawpack/__init__.py'>
>>> from clawpack import pyclaw
>>> pyclaw
<module 'clawpack.pyclaw' from '/home/ec2-user/clawpack/pyclaw/src/pyclaw/__init__.py'>
>>> from clawpack.geoclaw import topotools
>>> topotools
<module 'clawpack.geoclaw.topotools' from '/home/ec2-user/clawpack/geoclaw/src/python/geoclaw/topotools.py'>

>>> from clawpack.clawutil import whichclaw

`import clawpack` imports from:
    /home/ec2-user/clawpack

The CLAW environment variable is set to: 
    /home/ec2-user/clawpack
The PYTHONPATH environment variable is set to: 
    /home/ec2-user/clawpack

The following directories on sys.path might contain clawpack,
and are searched in this order:
    /home/ec2-user/clawpack

The following easy-install.pth files list clawpack:
>>>

It's a mystery to me how it finds things...

@ketch
Copy link
Member Author

ketch commented Nov 26, 2018

Thanks for the replies. I didn't view this as "giving up"; more of a "let each person work the way they prefer". And I don't think that separating the packaging of PyClaw and Riemann from AMRClaw and GeoClaw would cause problems for users of the latter packages -- they would still install PyClaw and Riemann as they do now (either with pip or by putting the code on their PYTHONPATH manually).

In any case, now that I have seen exactly what @rjleveque is doing, I think that the new installation process (without symlinks) does exactly what @mjberger and @rjleveque want. You can just clone clawpack, set your PYTHONPATH to point to it, and go. No need to run pip or python setup.py install. I guess this is what @rjleveque had discovered, but now I think I can explain why it works.

If you look at clawpack/clawpack/__init__.py, there is code there that adds the locations of all the Python code directories (for AMRClaw, Geoclaw, PyClaw, etc.) to the path. We did this so that pip could find them at install time, but this also allows Python more generally to find those subdirectories at runtime.

So I think clawpack/clawpack#141 should make everyone much happier after all?

@rjleveque
Copy link
Member

Great! Yes, now I understand why it is finding them from __init__.py.

So yes, it seems like this is all we needed all along instead of the sym-links to have things work smoothly for either pip or PYTHONPATH.

Good outcome!

So I think it's fine to merge clawpack/clawpack#141, once the merge conflicts are fixed.

Regarding this PR, I'll take a pass at tweaking the docs some more to try to explain both options, recommending pip in general but also saying how to set PYTHONPATH (with pointers to the webpage saying why this might be a bad idea), if that's ok.

@ketch
Copy link
Member Author

ketch commented Nov 26, 2018 •
edited

That sounds good. I'll work out the conflicts on clawpack/clawpack#141.

@rjleveque rjleveque mentioned this pull request Dec 16, 2018
Merged
@rjleveque
Copy link
Member

I am closing this in favor of #200, where I built on this PR and added some more discussion.

@rjleveque rjleveque closed this Dec 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@ketch @mjberger @rjleveque @mandli