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

Access the mpas_analysis package with an "entry point" #477

Merged
merged 10 commits into from
Nov 15, 2018

Conversation

xylar
Copy link
Collaborator

@xylar xylar commented Nov 8, 2018

This means that the run_mpas_analysis script has been removed and its functionality has been moved to mpas_analysis/__main__.py. When mpas_analysis is installed as a conda package, there will be an mpas_analysis script in the path (as well as a download_analysis_data script) in the path. When users want to run the analysis from the repo rather than the package, they will need to use:

python -m mpas_analysis ...

instead of ./run_mpas_analysis .... Nothing else about execution has changed (e.g. all flags work as before).

The documentation, comments, scripts and example config files have been updated to reflect the change.

Reason for this change

In order to move over to conda-forge instead of building the mpas_analysis package ourselves for each release, we need to have an entry point rather than a script. An entry point also allows us to call the main script the same thing as the package (mpas_analysis), which is less clunky than adding "run" to the front just to prevent conflicts.

closes #452

@xylar
Copy link
Collaborator Author

xylar commented Nov 8, 2018

I have tested this with a simple QU240wLI test case using the repo (i.e. python -m mpas_analysis ...). I haven't yet tested installing this locally as a conda package but will do so shortly.

xylar added 6 commits November 8, 2018 00:35
To call run the analysis, now do:
python -m mpas_analysis ...

Once built, the package can be run with:
mpas_analysis ...

download_analysis_datay.py now points to a function in __main__.py
These include config.default, AnalysisTask and its test
@xylar
Copy link
Collaborator Author

xylar commented Nov 8, 2018

It should be possible to do a test install of the conda package built from this branch using:

conda create -n mpas_analysis_1.1_py3.6 -c conda-forge -c xylar python=3.6 mpas_analysis=1.1
conda activate mpas_analysis_1.1_py3.6
mpas_analysis config.<run>

It isn't recognized as an installed package by setuptools
@xylar
Copy link
Collaborator Author

xylar commented Nov 8, 2018

After a few rebuilds (removing nco and adding some missing XML files to setup.py), the conda package seems to be working, too.

@xylar
Copy link
Collaborator Author

xylar commented Nov 9, 2018

@jhkennedy, if you would be willing to take a look at this when you have time, I think it's ready for a review. If you want to actually test it on a run, there are example runs pointed to in the configs/<machine> directories. If you'd rather do a review at the level of looking over the code, that's perfectly fine.

@milenaveneziani, would you be willing to run a test, once with the git repo (as usual) using the python -m mpas_analysis and once by creating a new conda environment? If you need help with the latter, let me know and maybe we can work on it next week. Make sure you don't try to add your conda environment to the e3sm-unified base install of miniconda, please.

@jhkennedy
Copy link

@xylar with a cursory glance it looks great! I'll look at it in depth tomorrow -- I should be able to test it on a run.

@milenaveneziani
Copy link
Collaborator

yes, I'd really like to run a couple of tests.
For the second one, with the conda package, I do have my own conda I can use.

Copy link

@jhkennedy jhkennedy left a comment

Choose a reason for hiding this comment

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

@xylar Everything looks pretty good to me!

I just have some minor comments:

@xylar
Copy link
Collaborator Author

xylar commented Nov 12, 2018

Thanks @jhkennedy, those are helpful comments and very easy to address.

  • We've tested and are supporting python 2.7, 3.5, 3.6 and 3.7 (I'm about to remove 3.5 from CI and switch to 3.7 instead so I'll likely keep 3.5 out of the list, just in case)
  • I'll add the entry point to the conda recipe and rebuild the package. Doesn't hurt to do this early.

This merge also adds the entry point to the conda recipe
@milenaveneziani
Copy link
Collaborator

@xylar: I have been trying to run MPAS-Analysis on cooley but I am having trouble (I will report on the changes I had to make in order for the batch job script to work once I am successful..). Please bear with me for another day or so. thanks.

@milenaveneziani
Copy link
Collaborator

@xylar: MPAS-Analysis is finally working again for me on cooley (although there are some changes that we need to apply to the batch job script: I will open a quick PR for this).
This is with using e3sm-unified and the python -m mpas_analysis command.

Now I want to use it as a conda package, but before doing that: is there an easy way to use/install the e3sm-unified packages in my conda environment? Or do I just list all packages under e3sm-unified and install them under my conda?

@milenaveneziani
Copy link
Collaborator

although there are some changes that we need to apply to the batch job script: I will open a quick PR for this

Or should I just push the new job_script.cooley.bash to this branch?

@xylar
Copy link
Collaborator Author

xylar commented Nov 15, 2018

@milenaveneziani,

Or should I just push the new job_script.cooley.bash to this branch?

As long as you think this PR is close to being merged, let's just update the Cooley job script in this PR.

Now I want to use it as a conda package, but before doing that: is there an easy way to use/install the e3sm-unified packages in my conda environment? Or do I just list all packages under e3sm-unified and install them under my conda?

This might be easiest to discuss in person. I'll come by really quick.

@milenaveneziani
Copy link
Collaborator

@xylar: the conda package works just fine. I installed it with no problems in my own ocnda.
The only warning message I get when running mpas_analysis... is this:

/lus/theta-fs0/projects/ClimateEnergy_2/milena/miniconda2.7/envs/mpas_analysis_1.1_py3.6/lib/python3.6/importlib/_bootstrap.py:219: RuntimeWarning: numpy.dtype size changed, may indicate binary incompatibility. Expected 96, got 88
  return f(*args, **kwds)

don't know if you care. From my point of view, you can merge this.

ps: there was already a config file available for the high-res on alcf, so we are good about it. I just updated it for the parallelTasks issue.

@xylar
Copy link
Collaborator Author

xylar commented Nov 15, 2018

@milenaveneziani, thanks so much for your careful testing! The warning you saw is really annoying but nothing to worry about. I suspect that it has to do with building numpy with one library and running it with another but I see that warning on every machine I use with the latest version of numpy.

@xylar xylar merged commit 9846447 into MPAS-Dev:develop Nov 15, 2018
@xylar xylar deleted the add_entry_point branch November 15, 2018 20:44
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.

change run_mpas_analysis to mpas_analysis entry point
3 participants