E3SM-Project / ChemDyg

Chemistry Diagnostics Package
BSD 3-Clause "New" or "Revised" License
0 stars 2 forks source link

Organize as python package #1

Closed xylar closed 1 year ago

xylar commented 1 year ago

@tangq and @golaz, here would be my suggestion for how to reorganize this into a python package (which could also eventually be a conda package).

xylar commented 1 year ago

I have verified that I can run the following locally. First, install Mambaforge for Linux x86_64 https://github.com/conda-forge/miniforge#mambaforge. You probably don't want to let it alter your .bashrc.. Assuming it's in ~/mambaforge:

source ~/mambaforge/etc/profile.d/conda.sh
source ~/mambaforge/etc/profile.d/mamba.sh
mamba create -y -n zppy_dev python=3.11 zppy=2.2.0
mamba activate zppy_dev
python -m pip install .

After that, I can see that the file are in the conda environment:

$ cd ~/mambaforge/envs/zppy_dev/lib/python3.11/site-packages/zppy_e3sm_chem_diags
$ ls
e3sm_chem_diags.py  __init__.py  __pycache__  templates
$ ls templates
chem_summary_table.bash            e3sm_pres_lat_plots.bash
e3sm_chem_cmip_comparison.bash     __init__.py
e3sm_chem_diags.bash               o3_hole_diags.bash
e3sm_chem_diags_ts.bash            __pycache__
e3sm_chem_index.bash               STE_flux_diags.bash
e3sm_chem_noaa_co_comparison.bash  surf_o3_diags.bash
e3sm_lat_lon_plots.bash            temperature_eq_plot.bash
e3sm_nox_emis_plots.bash           TOZ_eq_plot.bash
xylar commented 1 year ago

The next step would be to modify https://github.com/E3SM-Project/zppy/pull/402 to work with this. My suggestion would be that, rather than giving a path like the following for the plugin:

/home/xylar/mambaforge/envs/zppy_dev/lib/python3.11/site-packages/zppy_e3sm_chem_diags/e3sm_chem_diags.py

you would simply give the plugin name (plugin = zppy_e3sm_chem_diags) and zppy would find it using importlib.resources.

tangq commented 1 year ago

@xylar , thanks for making ChemDyg a python package and suggestions for eventually including it in e3sm_unified. None of our chemistry team member is familiar with this process, but I am sure that it's in good hands (you, @golaz , and @chengzhuzhang). Please feel free to add necessary changes.

xylar commented 1 year ago

@tangq, that's precisely my concern. I don't have the time, and I suspect neither do @chengzhuzhang and @golaz to become responsible for this package. Ut is my feeling that someone from the chemistry team needs to put the time in to develop this expertise. This isn't a service the infrastructure team can provide.

tangq commented 1 year ago

@xylar , I agree that it would be better that the chemistry team have this expertise. Let's talk about this at our meeting next week.

tangq commented 1 year ago

@xylar , @chengzhuzhang , @golaz, to follow up on what we discussed at the infrastructure meeting, we need to complete these steps (Please make corrections if needed) for including ChemDyg in e3sm_unified:

  1. Make ChemDyg a python package. (Done by @xylar as described above. Thanks!)
  2. Submit the ChemDyg package to the conda-forge channel. (To be done)
  3. Modify zppy plugin to work with this and use importlib.resources to find the path of plugins. (To be done)

In this ChemDyg space, I think we should focus on 2 and leave the discussions on 3 in the zppy space.

I found this conda-forge instruction page and it seems that the next step for 2 is to create the recipe file (meta.yaml) using graskull. I will try that later today.

xylar commented 1 year ago

@tangq, I'm happy to create the recipe. Grayskull won't help in this case. It's very handy for packages already available on PyPI, the server for the pip installation tool. But we won't use PyPI so the meta.yaml needs to be created from scratch.

You do need to release the package, since conda-forge recipes need to refer to a release version of the python package.

tangq commented 1 year ago

@xylar , if I create a release following these instructions, it will be based on the current main without the commits you made in this PR to make it a python package. Shall we merge this PR before the release, which will be referred to by the recipe?

xylar commented 1 year ago

@tangq, yes, those are the right instructions, but we would want to merge this PR first.

Let me rebase to pick up your recent changes. Then, you and @hsiangheleellnl can review.

xylar commented 1 year ago

Okay @tangq and @hsiangheleellnl, this is ready for review. I will make a few notes for your to respond to.

xylar commented 1 year ago

@tangq and @hsiangheleellnl, one more thing. I wouldn't do a release or create a conda package just yet. I think it makes sense to first get a release of zppy that fully supports plugins. The release of this package should depend on a version of zppy that can make use of it.

In the meantime, you can install zppy following the instructions for developers: https://e3sm-project.github.io/zppy/_build/html/main/getting_started.html#dev-env Then, you can go to your local clone of this repository and run:

python -m pip install -e .

This will install this python package (zppy-e3sm-chem-diags or e3sm-chem-diags or whatever we decide to call it) into the zppy conda environment in "editable" mode. That means that zppy will see the version of the code in your local clone, so if you make changes you don't need to rerun the install command for zppy to make use of those changes.

This is the typical way we would develop 2 python packages at the same time.

tangq commented 1 year ago

@xylar , thanks a lot. I will go through your notes tomorrow.

Okay @tangq and @hsiangheleellnl, this is ready for review. I will make a few notes for your to respond to.

chengzhuzhang commented 1 year ago

@xylar Thank you so much for your comments and note here. I found them very helpful. And I will plan to summarize the key steps into a confluence page, so other developers such as @susburrows who are interested in contributing new packages can follow.

xylar commented 1 year ago

Okay, I've made your 2 changes @hsiangheleellnl. I think that answers my questions for now. Please merge when you're happy.

tangq commented 1 year ago

This is very good tip - Thank you, @xylar .

@tangq and @hsiangheleellnl, one more thing. I wouldn't do a release or create a conda package just yet. I think it makes sense to first get a release of zppy that fully supports plugins. The release of this package should depend on a version of zppy that can make use of it.

In the meantime, you can install zppy following the instructions for developers: https://e3sm-project.github.io/zppy/_build/html/main/getting_started.html#dev-env Then, you can go to your local clone of this repository and run:

python -m pip install -e .

This will install this python package (zppy-e3sm-chem-diags or e3sm-chem-diags or whatever we decide to call it) into the zppy conda environment in "editable" mode. That means that zppy will see the version of the code in your local clone, so if you make changes you don't need to rerun the install command for zppy to make use of those changes.

This is the typical way we would develop 2 python packages at the same time.

xylar commented 1 year ago

@tangq and @hsiangheleellnl, are we agreed that the package and directory are to be named chemdyg? @tangq commented that this seems redundant (chemdyg inside of ChemDyg) but this is very common for python packages and just something you have to get used to.

Are we also agreed that (at least for now) e3sm_chem_diags.py should become chemdyg.py?

xylar commented 1 year ago

I went ahead with the change for now. I can always undo or fix the commit based on your feedback but it is late here so I want to give you the chance to decide today if you want to.

tangq commented 1 year ago

Yes, these changes are all good to me. Thanks.

@tangq and @hsiangheleellnl, are we agreed that the package and directory are to be named chemdyg? @tangq commented that this seems redundant (chemdyg inside of ChemDyg) but this is very common for python packages and just something you have to get used to.

Are we also agreed that (at least for now) e3sm_chem_diags.py should become chemdyg.py?

tangq commented 1 year ago

As @xylar recommended above we would wait until zppy supports this plugin to create a release or a conda package for chemdyg. But is this PR ready to be merged now? If so, I will click on the Squash and merge button.