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

Testing: tc4 installs python-netCDF4 via venv #1166

Merged
merged 12 commits into from
Jul 27, 2020
Merged

Testing: tc4 installs python-netCDF4 via venv #1166

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
56bad8b
Testing: tc4 installs python-netCDF4 via venv
marshallward Jul 22, 2020
c6eaddc
Testing: remove Python numpy and netCDF4 modules
marshallward Jul 22, 2020
7c528e8
Test: Install virtualenv for Python2
marshallward Jul 22, 2020
f3b148d
Test: Add python-dev for ARM64 numpy
marshallward Jul 22, 2020
e41a4fe
Test: Revert to Py3 for tc4 generation scripts
marshallward Jul 22, 2020
83115b8
Test: Adding python3-venv to Travis
marshallward Jul 23, 2020
23b8331
Test: Arm64 tc4 Configuration support
marshallward Jul 23, 2020
ac0c174
Test: tc4 common Python local-env
marshallward Jul 23, 2020
5e74250
Test: Use '.' in place of 'source' for Make
marshallward Jul 23, 2020
dfcdd1f
Test: Test for required Python modules in tc4
marshallward Jul 23, 2020
09c8c80
Test: Explicit python execs for tc4 input build
marshallward Jul 23, 2020
da5938e
Test: Setup Python venv at build time
marshallward Jul 24, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions .testing/tc4/Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,6 @@
ocean_hgrid.nc topog.nc temp_salt_ic.nc sponge.nc:
python build_grid.py
python build_data.py
python2 -m virtualenv local-env
Copy link
Collaborator

Choose a reason for hiding this comment

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

Convenient those these are for a user anything that is platform specific (these do not work everywhere) needs to be higher up. I think the local-env creation belongs in the .travis.yml file so we can put something different in the .gitlab-ci.yml file.

What you could do is move this step into it's own target for .testing/local-env and make these targets depend on .testing/local-env. That way it can be created by the travis/gitlab scripts but it will also be created for a user's convenience. It just won't work on gaea or everywhere.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, that is a problem. It seems like python3 -m venv ${dir} works on Gaea but python2 -m virtualenv ${dir} does not? I agree this needs to be resolved.

But I don't think the python3 -m venv local-env step (or py2 equivalent above) belongs in .travis.yml. This is not much different from apt install python-netcdf and arguably solves nothing, replacing one dependency with another.

I'm not really worried about Travis or Gaea's Gitlab, since those are controlled environments already. I'm more concerned about the user who has neither one set up.

Arguably the current scripts build_grid.py and build_data.py are also platform-specific, since they assume numpy and python-netCDF4 exist on the platform. So I concede that this PR really just shifts the requirements around without solving the problem.

We cannot call pip install netCDF without also calling virtualenv (or conda or whatever). So this command should only be called in conjunction with python -m venv local-env.

One suggestion is to do some autoconf-like testing to determine which one works.

But maybe there is just no way to do this unless the user has numpy and python netCDF4 installed.

Copy link
Collaborator Author

@marshallward marshallward Jul 22, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Other thoughts:

  • I had mistakenly thought virtualenv was in the Standard Library. It is not, which is arguably the source of our problems.
  • However, venv is part of the Python 3 Standard Library. So if we can assume Python 3, then we can assume venv.
  • So I suggest we do a bit of testing before running this Makefile:
    1. Check if python 3 is available. If so, no worries.
    2. Check if python 2 is being used. If so, then
      a. Test for virtualenv. If so, then replace venv with virtualenv
      b. If not, then test for python-netCDF4. If so, then don't even bother running virtualenv.
    3. Finally, if there is no 3 and 2 is unsuitable (for the reasons above) then we abandon the test.

While 2.x is still pervasive, 2.7 has been tagged for deprecation and its days are numbered. It won't be long until everyone is on Python 3. So I think we go with 3 using the proposed (and amended) solution above.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Fair enough. I don't disagree that we want this to work for a user but the current form will be hard to make work in our pipeline. If you do this we can work with it:

local-env:
	python2 -m virtualenv local-env \
	&& . local-env/bin/activate \
	&& pip install netCDF4
ocean_hgrid.nc topog.nc temp_salt_ic.nc sponge.nc: local-env
	. local-env/bin/activate \
	&& python build_grid.py \
	&& python build_data.py
```
because we can pre-build tc4/local-env.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think just switching to Python 3 would solve our problems (after adding it to the Travis instance)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh BTW I agree on splitting the rule, I had meant to do that at some point.

. local-env/bin/activate \
&& pip install netCDF4 \
&& python build_grid.py \
&& python build_data.py
2 changes: 1 addition & 1 deletion .travis.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ addons:
- tcsh pkg-config netcdf-bin libnetcdf-dev libnetcdff-dev gfortran
- mpich libmpich-dev
- doxygen graphviz flex bison cmake
- python-numpy python-netcdf4
- python-dev python-virtualenv
- bc

jobs:
Expand Down