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

Pip install #55

Merged
merged 29 commits into from
Dec 23, 2015
Merged

Pip install #55

merged 29 commits into from
Dec 23, 2015

Conversation

dkirkby
Copy link
Member

@dkirkby dkirkby commented Dec 17, 2015

Motivated by some discussion on #37.

@dkirkby
Copy link
Member Author

dkirkby commented Dec 17, 2015

Travis checks are failing on this branch and it looks like the problem is with import specter.

Someone will need to resolve merge conflicts on this branch (probably due to #50).

I tried the following install:

pip install --upgrade svn+https://desi.lbl.gov/svn/code/desimodel/branches/pip-install#egg=desimodel

followed by:

python setup.py develop

from the current master branch of desisim. There were no errors during the installations, but quickbrick now fails with:

...
  File "/Users/david/Cosmo/DESI/code/desisim/py/desisim/targets.py", line 11, in <module>
    from desimodel.focalplane import FocalPlane
ImportError: No module named focalplane

Running quickgen also fails, but I guess this is expected and should be resolved by #44:

...
  File "/Users/david/Cosmo/DESI/code/desisim/bin/quickgen", line 23, in <module>
    import desimodel.simulate as sim
ImportError: No module named simulate

@weaverba137
Copy link
Member

I think I can easily fix up the dependency installs to get Travis working.

@weaverba137
Copy link
Member

I'm having some trouble getting Travis to install dependencies in the correct order. For example, desiutil is needed by just about everything, and it looks like pip is downloading it first, but not installing it first. Is there a way to force pip to install before resolving other requirements?

@rainwoodman
Copy link

Add one more pip install line?
On Dec 18, 2015 12:32 PM, "Benjamin Alan Weaver" notifications@github.com
wrote:

I'm having some trouble getting Travis to install dependencies in the
correct order. For example, desiutil is needed by just about everything,
and it looks like pip is downloading it first, but not installing it
first. Is there a way to force pip to install before resolving other
requirements?


Reply to this email directly or view it on GitHub
#55 (comment).

@weaverba137
Copy link
Member

This is bad. Installing numpy, scipy, astropy in the bare Travis/Python environment is giving me many problems. Stay tuned for further head-against-wall bashing.

@weaverba137
Copy link
Member

OK, we are now at the point where this branch needs to incorporate changes in order to eliminate all dependencies on desimodel, except desimodel.io. In other words, the tests are currently failing when it tries to load desimodel.focalplane.

@sbailey
Copy link
Contributor

sbailey commented Dec 18, 2015

I'll work on taking out the FocalPlane dependency. I'll use specsim.transform for now, while noting:

  • it uses a constant platescale which is incorrect for desi
  • it seems that focal plane transformations belong in a core package, not a simulation package

desisim just uses this to get approximately correct RA, dec distributions for targets assigned to fibers so this isn't a big deal, but we should expect further refactors along this line in the future.

@weaverba137
Copy link
Member

It might be worthwhile to merge this PR now, even though tests are failing, because this branch was intended to add Travis tests to master. That's why there are no Travis tests on master, or any other branch.

@dkirkby
Copy link
Member Author

dkirkby commented Dec 22, 2015

Ok with me, unless @sbailey is still working on his FocalPlane migration.

@sbailey
Copy link
Contributor

sbailey commented Dec 22, 2015

I'm working on FocalPlane / desimodel this afternoon; give me a few more hours. I'm leaning toward putting FocalPlane back into the pip-install branch of desimodel itself, since it seems that is where it really belongs, rather than desisim or desispec. If I'm too distracted to finish by the end of today, then we can punt on that and merge anyway.

@weaverba137
Copy link
Member

Based on discussions on the telecon today, we're going to move forward with testing this against the trunk of desimodel which still includes all the subpackages, so stay tuned for that.

@weaverba137
Copy link
Member

Just a reminder: desimodel/trunk still has a dependency on fitsio, so that has to be installed to get desimodel to work.

@sbailey
Copy link
Contributor

sbailey commented Dec 22, 2015

I fixed this in desimodel; go ahead and remove the fitsio dependency here. Note: desimodel/bin scripts still use fitsio for historical reasons, but those aren't needed/used by desisim and we can refactor those to use astropy too. I think I have expunged fitsio from any of the desimodel/py/desimodel modules which should be good enough for the purposes of this PR (non-)dependency.

@weaverba137
Copy link
Member

OK, @sbailey, I just saw that. I'll pick up the latest from desimodel/trunk.

@weaverba137
Copy link
Member

OK, the latest commit can now install everything, and actually start running the tests. The remaining failures appear to fall into these categories:

  1. The $DESIMODEL environment variable is not set.
  2. The $DESI_BASIS_TEMPLATES environment variable is not set.
  3. The $DESI_ROOT environment variable is not set.

I know how to fix the missing environment variables in a unit test environment, but getting the actual data that those environment variables point to is another story. Is it fair to have unit tests fail because data that is outside of the package is missing?

I don't know.

I do know that astropy's answer is that it is unfair, and that tests that require external data should be skipped in routine (i.e. Travis) tests, or the tests get redirected to use small, stand-in data files that are included with the package.

@weaverba137
Copy link
Member

One additional comment: I've noticed recently that errors due to missing environment variables are really tricky to track if you use os.getenv() to read them. If the environment variable is not set, os.getenv() returns None, and the code goes on its merry way. If you use os.environ[] instead, then a missing environment variable will raise a KeyError, and so will be very easy to track down.

@sbailey
Copy link
Contributor

sbailey commented Dec 22, 2015

Regarding tests failing due to lack of input files:

That is the core reason why the tests existed but has not yet been merged into Travis. We have not yet done the work to trim down the templates into lightweight test-only versions. Given that those template files themselves are still evolving, it could be a little meaningless to not use them if available — i.e. the Travis tests should only fall back to downloading (as yet not existing) test-only versions if the full versions aren't available, and we should have a programmatic way of generating the lightweight versions from any future updated template files. That might be as simple as just writing out the first 3 basis templates from a larger template file.

Perhaps the pragmatic solution for today is to add some @unittest.skipif logic so that it doesn't bork Travis, but it will very significantly reduce the actual coverage of the Travis tests.

@weaverba137
Copy link
Member

We are passing!

weaverba137 pushed a commit that referenced this pull request Dec 23, 2015
@weaverba137 weaverba137 merged commit 497e2e9 into master Dec 23, 2015
@weaverba137 weaverba137 deleted the pip-install branch December 23, 2015 04:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants