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

added pyrad_mch dependency #46

Merged
merged 3 commits into from
Aug 19, 2022
Merged

added pyrad_mch dependency #46

merged 3 commits into from
Aug 19, 2022

Conversation

wolfidan
Copy link
Contributor

Hi everybody,

As asked in the github issues, here is the addition of pyrad_mch as dependency in both environment.yml files.
Thanks,
Daniel

@github-actions
Copy link

github-actions bot commented Aug 16, 2022

👋 Thanks for opening this PR! The Cookbook will be automatically built with GitHub Actions. To see the status of your deployment, click below.
🔍 Git commit SHA: 26dc6cb
✅ Deployment Preview URL: https://openradar.github.io/erad2022/_preview/46

@kmuehlbauer
Copy link
Collaborator

@wolfidan The problem with pyart_mch is, that no Python3.9 version exists on conda-forge. It looks like there are also build issues for Python3.9 over at conda-forge at that feedstock. https://github.com/conda-forge/pyrad_mch-feedstock

@wolfidan
Copy link
Contributor Author

@kmuehlbauer , thanks for pointing that out! Our pyrad CI works for python3.9, so I didn't realize this failure on azure in the conda package for 3.9. I will sort it out asap.

@kmuehlbauer
Copy link
Collaborator

@wolfidan Yes, the 1.3.0 version is still an unmerged PR in the feedstock. If this one get's merged we should have the artifacts ready in some hours.

@wolfidan
Copy link
Contributor Author

Sorry as pointed out by @kmuehlbauer (thanks again!) there are some issues in the conda builds at the moment, also because numpy.distutils apparently just got deprecated and we still use it in our setup.py. I'm doing my best to fix this.

@mgrover1
Copy link
Collaborator

Will this create conflicts with:

import pyart

Since it requires pyart_mch? Will it override the arm_pyart installation?

@wolfidan
Copy link
Contributor Author

I modified it to make sure it also works with arm_pyart. So it should just use the version that is available. But yes we need to make sure it does not install pyart_mch through conda. I'm pretty sure it does not but I will double check.

@wolfidan
Copy link
Contributor Author

@kmuehlbauer , thanks for your proposition to help me. I managed to make it run locally with setuptools but on my github actions I get this error: https://github.com/MeteoSwiss/pyrad/runs/7861917497?check_suite_focus=true

Strange because when I compile locally I don't get this problem, neither with
python setup.py install
nor with
pip install .

Everything I tested now didn't help and I can't find much on the web. Usually this error happens when you try to import your package while running python inside the package installation folder, but that cannot be the case here.

@kmuehlbauer
Copy link
Collaborator

Omg, please have a look here

pypa/setuptools#3532

@kmuehlbauer
Copy link
Collaborator

Maybe it just helps to pin setuptools?

@wolfidan
Copy link
Contributor Author

wolfidan commented Aug 16, 2022

Thanks, I managed to solve it finally, it was just a matter of modifying find_packages of setuptools, before that it only included the main module but not the submodules, which caused the issue.

It was kind of misleading, starting a new conda environment from scratch, I realized that in fact it did not compile correctly locally as well.

@wolfidan
Copy link
Contributor Author

I finally managed to make a new working (also with python3.9) pyrad conda package. Still want to run a test or two before we merge, though.

But I'm still struggling with pyart_mch, and even though it's not needed for the course, I'm feeling bad having this half broken package. It works locally and on pip but the conda feedstock fails. I watched the arm_pyart feedstock and it does not seem to try to compile the cython files at this stage of the process, so this might be my problem.

I have no idea what's going around here, been trying things for hours. Maybe @mgrover1 , would have a minute to check out the conda feedstock log, maybe you have experienced similar things in your transition to setuptools? That would be extremely helpful, thank you :)

@wolfidan
Copy link
Contributor Author

And I had another question for you python experts @kmuehlbauer @mgrover1, I realized that conda considers pyart_mch as a dependency of pyrad_mch which is bad.

But this is probably (?) due to the fact that pyart_mch is currently a git submodule of pyrad_mch. So I guess the only way to let the user choose between arm_pyart and pyart_mch would be to remove pyart_mch from the submodules of pyrad_mch? In that way it would not be installed automatically with conda install and the user could choose whatever fork he wants. Or is there another way?

@mgrover1
Copy link
Collaborator

@wolfidan There were some big changes with setuptools and conda-forge this past week... trying to sort out the issues on this end too.

@kmuehlbauer
Copy link
Collaborator

How do we want to resolve this PR? @wolfidan is this correct, that we can't install pyart_mch and arm_pyart into the same conda environment? How should this be tackled?

@wolfidan
Copy link
Contributor Author

@kmuehlbauer , I just made a new conda package that should hopefully solve the problem. However when I try to install on a new environment it still gets the old one. I guess I have to wait some time until the repo is updated. So I'll keep you posted.

Otherwise there's also the --no-deps argument in conda install that would only install pyrad and nothing else but I don't know if it's possible to specify this in the binder.

@kmuehlbauer
Copy link
Collaborator

@wolfidan It takes some time for the package to get distributed to CDN.

We can move the installation part to appendix.txt maybe and install pyrad there.

@wolfidan
Copy link
Contributor Author

@kmuehlbauer , I could now download the latest version using conda install -c conda-forge pyrad_mch and I can confirm that on a fresh conda env it did NOT install Py-ART. So I think we're fine to add it in the binder.

@jfigui
Copy link
Collaborator

jfigui commented Aug 19, 2022

Hi @wolfidan,

I am not sure I understand what you are trying to do here. In the past we had a pyrad_mch and a pyrad_arm conda packages. Pyrad_mch had pyart_mch as a dependency and pyrad_arm had arm_pyart as a dependency. In practice they were two completely independent conda packages even though the python package pyrad was unique. What would be the rationale to change that? The idea was that each pyrad package was serving at two independent communities.

@wolfidan
Copy link
Contributor Author

@jfigui , yes you're right, since it is now homogeneized for whatever pyart version, I realized we can actually generate the two packages with the same pip version just by changing the dependencies in the conda feedstock.

So I now have the two packages
https://anaconda.org/conda-forge/pyrad_mch
https://anaconda.org/conda-forge/pyrad_arm

Which have arm_pyart and pyart-mch resp. as dependencies.

I will change this PR in an hour or so (after the repos are updated and I can test) to use pyrad_arm instead of pyart_mch.

binder/environment.yml Outdated Show resolved Hide resolved
environment.yml Outdated Show resolved Hide resolved
@mgrover1
Copy link
Collaborator

@wolfidan @kmuehlbauer should we go ahead and merge?

@kmuehlbauer
Copy link
Collaborator

Yes, let's see if anything breaks. The earlier we know the better.

@mgrover1
Copy link
Collaborator

Yes, let's see if anything breaks. The earlier we know the better.

Agreed! Merging away!

@mgrover1 mgrover1 merged commit 4bdeecb into openradar:main Aug 19, 2022
github-actions bot pushed a commit that referenced this pull request Aug 19, 2022
@wolfidan
Copy link
Contributor Author

Thanks for the merge, sorry I was away a few hours.

@mgrover1
Copy link
Collaborator

No worries!

@kmuehlbauer
Copy link
Collaborator

@mgrover1 @wolfidan It looks like everything is good. We would need to link the pyrad notebooks with the TOC and maybe move them into the notebooks folder.

@mgrover1
Copy link
Collaborator

Yup! That makes sense.

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.

4 participants