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

1st draft README with install instructions. #62

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

truth-quark
Copy link
Collaborator

The PR expands the README & adds a first draft of instructions for setting up virtual environments.

It'd be great if someone could run through the Mac instructions to see if:

  • They make sense
  • The install process works
  • Tests run locally
  • Tests + coverage run locally

@truth-quark truth-quark added the documentation Improvements or additions to documentation label Aug 2, 2024
$ conda install pip
$
$ cd <your-um2netcdf-project-dir>
$ pip install -r requirements.txt

Choose a reason for hiding this comment

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

Which requirements are requiring us to use pip install (as opposed to conda install)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ooops, this might be a cut/paste/hasty typing error. I barely use conda, but I think this command is supposed to be conda install --file requirements.txt.
Are you a regular conda user?

Choose a reason for hiding this comment

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

Not a super-user, but I regularly try to stay away from pip if at all possible (conda gives better compartmentalization). The requirements.txt given won't play nice with conda due to some missing packages. We might consider setting up an environment.yaml instead that describes the required packages (from memory, that allows you to explicitly set which packages need to come in via pip).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Quick note: requirements.txt came from pip freeze from my local virtualenv (the get something documented 1st approach).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For what it's worth, the install "docs" were ripped from ACCESS-NRI/amami#13. The amami project included a version of um2nc, so the virtualenv dependencies are similar or the same.

From what I recall, getting an environment built using that method required the hacky conda & pip commands...

Choose a reason for hiding this comment

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

I'm still mulling the virtualenv vs conda question. Should we even be supplying/documenting a virtualenv install option? I think the world has pretty definitively shifted over to conda now (and besides, everything else I've seen so far at ACCESS-NRI appears to stick with conda).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Should we even be supplying/documenting a virtualenv install option? I think the world has pretty definitively shifted over to conda now (and besides, everything else I've seen so far at ACCESS-NRI appears to stick with conda).

I added virtualenv instructions because of the... old habit of using lower level tools locally for yeeeears.

There should be a working conda setup config via the newly added CI content. The docs could be rejigged to focus mainly on conda, assuming most folks are non-developers, simply needing to setup & run the code. virtualenv instructions could be hived off to a separate section for those needing to create an isolated Py environment. I don't know who the users are, so I'm unsure of the most useful approach to take.

Choose a reason for hiding this comment

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

I just discovered the pipreqs package. Running it over the code gives the following (much reduced) dependency list:

cf_units==3.2.0
cftime==1.6.4
f90nml==1.4.4
mule==2020.1.1
netCDF4==1.7.1
netCDF4==1.7.1.post2
numpy==2.1.1
pytest==8.3.2
scitools_iris==3.10.0

I'm pretty sure the versions shown correspond to what is currently installed in my environment, not what is actually required. All of these except cf_units and scitools_iris can be installed via conda; those two require pip.

I'm still having trouble making the tests run to see if this new 'minimal' environment would work, I managed to blow away my package import updates doing other things...

Choose a reason for hiding this comment

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

Got my imports to work! And confirmed that a bare conda environment with only the above requirements explicitly installed passes the test suite.

MarkupSafe==2.1.5
matplotlib==3.9.0
matplotlib-inline==0.1.7
mule @ git+https://github.com/metomi/mule@cce4b99c7046217b1ec1192118a786636e0d8e54#subdirectory=mule

Choose a reason for hiding this comment

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

Is this newer than what conda (2020.01.1 on the coecms channel) could provide?

Choose a reason for hiding this comment

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

To (partially) answer my own question, after commenting out the mule requirement (which conda doesn't understand), I was left with the following packages unable to be found:

  - xxhash==3.4.1
  - stack-data==0.6.3
  - pure-eval==0.2.2
  - matplotlib==3.9.0
  - antlr4-python3-runtime==4.7.2

The channel list for this attempt was:

 - accessnri
 - conda-forge
 - http://ssb.stsci.edu/astroconda
 - defaults
 - coecms

(noting that some complaint about coecms was made).

@@ -0,0 +1,55 @@
antlr4-python3-runtime==4.7.2

Choose a reason for hiding this comment

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

Can this be bought up to , e.g., 4.13.1 safely? I can see versions 4.8 thru 4.13.1 available via conda.

kiwisolver==1.4.5
locket==1.0.0
MarkupSafe==2.1.5
matplotlib==3.9.0

Choose a reason for hiding this comment

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

I was a bit stunned for this to be missing from conda, but a subsequent search shows 3.9.1 is available.

Suggested change
matplotlib==3.9.0
matplotlib==3.9.1

scitools-iris==3.9.0
shapely==2.0.4
six==1.16.0
stack-data==0.6.3

Choose a reason for hiding this comment

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

The last version of stack_data I can see on conda-forge is 0.6.2.

Suggested change
stack-data==0.6.3
stack-data==0.6.2

Choose a reason for hiding this comment

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

Could also put in a => constraint?

pluggy==1.5.0
prompt_toolkit==3.0.47
ptyprocess==0.7.0
pure-eval==0.2.2

Choose a reason for hiding this comment

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

conda thinks this is called pure_eval... weird. (The requestion version is available though.)

@truth-quark
Copy link
Collaborator Author

While experimenting with only required um2nc dependencies , I got all tests passing (@ b1539af) in a virtualenv with the deps below. matplotlib is not required as um2nc doesn't import modules requiring it, but it is a dep of scitools-iris.

antlr4-python3-runtime==4.7.2
Cartopy==0.23.0
certifi==2024.7.4
cf-units==3.2.0
cftime==1.6.4
click==8.1.7
cloudpickle==3.0.0
dask==2024.8.0
f90nml==1.4.4
fsspec==2024.6.1
iniconfig==2.0.0
locket==1.0.0
mule @ git+https://github.com/metomi/mule@cce4b99c7046217b1ec1192118a786636e0d8e54#subdirectory=mule
netCDF4==1.7.1.post2
numpy==1.26.0
packaging==24.1
partd==1.4.2
pluggy==1.5.0
pyproj==3.6.1
pyshp==2.3.1
pytest==8.3.2
python-dateutil==2.9.0.post0
PyYAML==6.0.2
scipy==1.14.0
scitools-iris==3.10.0
shapely==2.0.5
six==1.16.0
toolz==0.12.1
xxhash==3.4.1

Copy link

@marc-white marc-white left a comment

Choose a reason for hiding this comment

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

While experimenting with only required um2nc dependencies , I got all tests passing (@ b1539af) in a virtualenv with the deps below. matplotlib is not required as um2nc doesn't import modules requiring it, but it is a dep of scitools-iris.

I like having the 'barebones' requirements listed for the module in question, but do we need to consider the requirements that most users of the module will have? E.g., is there some analysis package that most users will want that will clash with one of the dependencies listed above?

```Bash
# works for Linux and MacOS
$ conda create -n ums
$ activate ums

Choose a reason for hiding this comment

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

Suggested change
$ activate ums
$ conda activate ums

# works for Linux and MacOS
$ conda create -n ums
$ activate ums
$ conda install pip

Choose a reason for hiding this comment

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

Suggested change
$ conda install pip
$ conda install -n ums pip

Choose a reason for hiding this comment

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

This could be added in the environment creation step:
conda create -n ums python=<3.10/3.11/3.12> pip


```
```Bash
$ cd <your um2nc-standalone dir>
$ pytest --cov-report=html --cov=umpost

Choose a reason for hiding this comment

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

I've made it to this point and hit a few snags:

  • The tests don't correctly load the source code for testing. I've made some changes to the import statements that appear to fix the issue.
  • There's reference in both the tests and the source to a module um2netcdf4 - however, the umpost "module" contains only a straight um2netcdf (missing the trailing 4). I'm assuming it's meant to be referencing the one in this code base?

I'm going to make a wild assumption that what's happened is this code is being tested on a system where there is already some version of things called umpost and um2netcdf4 installed, and pytest is actually testing those rather than the code in the repo...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, we had an odd temporary arrangement going:

  • I developed umpost/um2netcdf
  • Spencer developed the conversion driver script on gadi, using an environment with um2netcdf4 & testing against that

#80 covers an import switchover to umpost/um2netcdf, which got merged 5 minutes ago. Do you want to grab the latest develop branch & see if you can get a working env & run the tests?

Choose a reason for hiding this comment

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

I'm looking at the latest develop branch, but it's still angry about trying to find umpost:

ImportError while importing test module '/Users/marc/Documents/ACCESS-NRI/um2nc-standalone/test/test_um2netcdf.py'.
Hint: make sure your test modules/packages have valid Python names.
Traceback:
../../../anaconda3/envs/ums/lib/python3.12/importlib/__init__.py:90: in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
test/test_um2netcdf.py:5: in <module>
    import umpost.um2netcdf as um2nc
E   ModuleNotFoundError: No module named 'umpost'

Choose a reason for hiding this comment

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

Looks like the impasse may come from pytest vs python -m pytest - the latter works fine for me

https://docs.pytest.org/en/7.1.x/explanation/pythonpath.html#invoking-pytest-versus-python-m-pytest

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 wonder if that's due to python -m adding cwd to PYTHONPATH behind the scenes, whereas pytest alone doesn't do this. If I start a fresh terminal & activate the um2nc venv, I have to export/set the PYTHONPATH manually in that term session.

Choose a reason for hiding this comment

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

That's exactly the issue. There is a config setting for pyproject.toml that sets the pythonpath for pytest, but I can't seem to make it work properly for the current directory structure...

$ brew install udunits
```

if this fails with some compiler/header errors, try:
Copy link
Collaborator

Choose a reason for hiding this comment

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

brew install udunits ran without reporting any issues. It wasn't until trying pip install -r requirements.txt step complained about the missing environment variables:

      ValueError: Require to set UDUNITS2_XML_PATH for a cf-units wheel build.
      [end of output]
  
  note: This error originates from a subprocess, and is likely not a problem with pip.
  ERROR: Failed building wheel for cf-units
Failed to build cf-units
ERROR: ERROR: Failed to build installable wheels for some pyproject.toml based projects (cf-units)

Do you know if exporting the environment variables should be a required step before installing udunits?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ooops, my earlier cut/paste job of the instructions is deficient!

I originally added venv setup notes here:
ACCESS-NRI/amami#13 (comment)

The relevant tweak I needed to get a working venv was exporting these env vars:

# set up non standard cf-unit paths
export UDUNITS2_XML_PATH=/opt/homebrew/Cellar/udunits/2.2.28/share/udunits/udunits2.xml
export UDUNITS2_LIBDIR=/opt/homebrew/lib
export UDUNITS2_INCDIR=/opt/homebrew/Cellar/udunits/2.2.28/include

Let's see if that works...

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah yep, that worked for me! I'm wondering whether this should be included as required before the brew install udunits in the install, as the error I got was only raised later when installing the other packages, rather than when installing udunits?

Choose a reason for hiding this comment

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

This seems to be Python version dependent. I've been trying the instructions for setting up a conda environment, and this error only occurs for Py 3.12. Py 3.11 and 3.10 go through without apparent incident.


```Bash
# works for Linux and MacOS
$ conda create -n ums

Choose a reason for hiding this comment

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

Suggested change
$ conda create -n ums
$ conda create -n ums python=<3.10/3.11/3.12>

When I first tried this, conda gave me Py 3.9 for some reason...

Choose a reason for hiding this comment

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

This could also include pip:
conda create -n ums python=<3.10/3.11/3.12> pip

@marc-white
Copy link

I think some of the import issues etc. I'm seeing in this branch are fixed in #63 - can this branch get a merge update once that PR is merged in?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants