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

Let's get MintPy on conda-forge #648

Closed
jhkennedy opened this issue Aug 27, 2021 · 19 comments
Closed

Let's get MintPy on conda-forge #648

jhkennedy opened this issue Aug 27, 2021 · 19 comments

Comments

@jhkennedy
Copy link
Collaborator

jhkennedy commented Aug 27, 2021

We've got a recipe (well, recipes) to get MintPy v1.3.1 (and PyAPS3 and PySolid) on conda-forge mostly ready to go, but there are a few things that need to be resolved before we can open the PR to conda-forge/staged-recipes.

On the technical side:

  1. ✔️ the conda-forge build of pykml is out of sync with the PyPI package (and pip installing the repository via git+https),
  2. ✔️Conda-forge requires building from tarballs, not repos, and a lot of the CI/CD wrapped around conda-forge works best with published package (e.g., PyPI) or published releases, so we'd need a release of PyAPS3 in GitHub and/or a published version on PyPI
    • Also, I'd like to better understand the relationship between yunjunz/PyAPS and AngeliqueBenoit/pyaps3? I'm basing off of yunjunz/PyAPS because AngeliqueBenoit/pyaps3 appears abandoned and @AngeliqueBenoit doesn't seem to be around?
  3. ❌ Windows builds will have to wait for a new release (at least until Add doc on install MintPy on Windows #647 is released, but some other refactoring I'll expand on in a subsequent comment/issues would help significantly)

On the organizational side:

  • We'd like your blessing to put MintPy on conda-forge (I think we got it in this comment, but it's always good to double check)

  • We need to know who to list as a maintainers of the conda-forge recipe (there will be spam)? So far I have (GitHub ids):

    @yunjunz, I'm assuming you would like to be as well?
    Note: This is entirely optional -- we're willing to maintain the recipe even if no one from MintPy wants too.


I'd love it if the MintPy developers would take it for a spin and let me know if you encounter any problems. I've built a linux version using the conda-forge build process locally, and uploaded it to the hyp3 conda channel for testing (mac will need to wait until it's on conda-forge as working the conda-forge pipeline locally on mac is... hard). You can set up a new conda environment with MintPy installed (recommended) like:

conda create -n conda-mintpy -c conda-forge -c hyp3 python=3.8 mintpy
conda activate mintpy

Or install it into an existing environment with:

conda install -c conda-forge -c hyp3 mintpy

Either should set up all the necessary paths and environment variables just like if you followed the conda install instructions.

Importantly, MintPy's test directory, with test_smallbaselineApp.py will not be included in the conda package, so if you want to run those tests, you'll need to:

conda activate conda-mintpy
git clone https://github.com/insarlab/MintPy.git
cp -r MintPy/test ${MINTPY_HOME}/../

MintPy/test/test_smallbaselineApp.py

And test_smallbaselineApp.py will be executed in the conda-mintpy environment against the conda-forge build of MintPy.

@welcome
Copy link

welcome bot commented Aug 27, 2021

👋 Thanks for opening your first issue here! Please make sure you filled out the template with as much detail as possible. We appreciate that you took the time to contribute!
Make sure you read our contributing guidelines

@jhkennedy
Copy link
Collaborator Author

jhkennedy commented Aug 27, 2021

There's also a few things MintPy could do (that we can help with) to significantly improve the packaging and distribution of MintPy, most of which would simplify the conda-forge recipe and make windows support in particular a lot easier.

This will mainly boil down to:

  • test_smallbaselineApp.py and similar should look for configs relative to itself, not $MINTPY_HOME or mintpy.__file__
  • not printing on import:
    try:
    os.environ['MINTPY_HOME']
    except KeyError:
    print('Using default MintPy Path: %s' % (mintpy_path))
    os.environ['MINTPY_HOME'] = mintpy_path
  • Drop any reliance on manipulating environment variables for installation (PATH, PYTHONPATH especially; MINTPY_HOME as well ideally) by leveraging console_scripts entry_points in setup.py

I can expand on these in this issue, or in more focused issues/PRs -- I'm leaning towards more focused issues/PRs to keep this issue focused on "MintPy v1.3.1 on conda-forge and any testing feedback", but happy follow your preference.

@Alex-Lewandowski
Copy link
Contributor

Alex-Lewandowski commented Aug 27, 2021

I tried adding mintpy via conda to the insar_analysis env in OpenSARlab, replacing the old install in our script with the new conda install and adding the hyp3 channel. The env created successfully. Then I tried running notebooks/SAR_Training/English/Hazards/LosAngeles_time_series.ipynb

It hung up on !info.py inputs/ifgramStack.h5 with /bin/bash: /home/jovyan/.local/envs/mintpy/lib/python3.8/site-packages/mintpy/info.py: Permission denied

It looks like info.py doesn't have execute permissions:
image

@jhkennedy
Copy link
Collaborator Author

jhkennedy commented Aug 27, 2021

@Alex-Lewandowski oh you're right! pip doesn't preserve the executable-ness of package files. You should be able to get it running with a:

chmod ug+x ${MINTPY_HOME}/*.py

I've pushed a commit to fix that, and an error with the deactivation script. I'll push a new build sunday that will fix those errors.

I'll edit this comment once it's available, so until then, run the above command (once) after creating and activating the environment.

EDIT: The should be fixed now!

@jhkennedy
Copy link
Collaborator Author

@Alex-Lewandowski I just pushed another build; this should be fixed now!

@Alex-Lewandowski
Copy link
Contributor

Thanks @jhkennedy!

@Alex-Lewandowski
Copy link
Contributor

Just tested in OpenSARlab and it is running smoothly! I'll update the our conda environments to use it.

@yunjunz
Copy link
Member

yunjunz commented Aug 30, 2021

This is awesome. Thank you @jhkennedy and @Jlrine2 for making the long-desired conda-forge recipe happening!!!

On the organizational side:

  1. Yes, you have my full blessing!
  2. I would be happy to join the maintainer list of the recipe. I guess @hfattahi would like it too?

On the technical side:

  1. yunjunz/PyAPS is a fork of the AngeliqueBenoit/pyaps3 repo. I have been maintaining yunjunz/PyAPS repo to be used by mintpy since the other one is not actively maintained.

    • I could make a GitHub release on yunjunz/PyAPS so that the conda-forge recipe work could move forward.
    • The published version on PyPI should be based on the original repo AngeliqueBenoit/pyaps3. I will issue a PR to merge the changes, hopefully @AngeliqueBenoit will consider merge it and publish on PyPI.
  2. insarlab/PySolid has releases on GitHub. Unfortunately, the name has been registered on PyPI by another project, but it seems like this is not an issue?

I will test it later in the week and give you some feedback.

I have copied your suggestions of changes in mintpy to simplify the conda-forge recipe at #652.

@jhkennedy
Copy link
Collaborator Author

  1. yunjunz/PyAPS is a fork of the AngeliqueBenoit/pyaps3 repo. I have been maintaining yunjunz/PyAPS repo to be used by mintpy since the other one is not actively maintained.
    • I could make a GitHub release on yunjunz/PyAPS so that the conda-forge recipe work could move forward.
    • The published version on PyPI should be based on the original repo AngeliqueBenoit/pyaps3. I will issue a PR to merge the changes, hopefully @AngeliqueBenoit will consider merge it and publish on PyPI.

Okay, I'd say make a GitHub release for it (conda-forge work fine pointing at github releases), and if/when^ @AngeliqueBenoit merges your changes and publishes to PyPI, we could cut over to it easily.

Really, it might be worth proposing:

  • she adds you to the repo and adds you on PyPI as an owner so that you can develop/publish from her repo, or
  • she directly people to your fork and adds you as an owner to the project on PyPI so that you can develop/publish your repo,
  • or she moves her repo into the insarlab org and adds you as an owner to the project on PyPI so that mintpy-dev can develop/publish it
  1. insarlab/PySolid has releases on GitHub. Unfortunately, the name has been registered on PyPI by another project, but it seems like this is not an issue?

Yep, not an issue to point conda-forge at GitHub, and since that PyPI project seems dead, I don't think anyone will care about name conflicts (that pysolid isn't in conda-forge either).

Really, for both of these, the conda-forge reviewers will check on it and we can work with them for anything the give us feedback on.

So, as soon as you cut a PyAPS release, I can open the conda-forge PR!


^ I don't expect this to happen as AngeliqueBenoit/pyaps3#2 was opened 8 mo ago and it hasn't been touched, or commented on.

@yunjunz
Copy link
Member

yunjunz commented Sep 1, 2021

@jhkennedy here is the released version of PyAPS on yunjunz/PyAPS: https://github.com/yunjunz/PyAPS/releases/tag/v0.2.0.

Thank you for the constructive suggestions as well!

@jhkennedy
Copy link
Collaborator Author

Alright! I've opened the PR to get MintPy on conda forge! conda-forge/staged-recipes#16050

@yunjunz I've added you as a maintainer -- you'll just need to comment you're willing to be so on the PR.

@hfattahi , since I haven't heard from you, I didn't list you. Once the feedstock is available, you can request to be added if you want: https://conda-forge.org/docs/maintainer/updating_pkgs.html#updating-the-maintainer-list

@pbrotoisworo
Copy link
Contributor

pbrotoisworo commented Sep 4, 2021

Just a question. I noticed in the PR that PySolid for Windows will not be included for now. I've been using Windows for MintPy for a while (Windows 8). Does that mean that if I try to install MintPy for my Windows OS via conda-forge then MintPy will not work since it is missing PySolid?

EDIT: Sorry you may disregard this. I just reread the first post. It seems to have been updated and answered my question.

@jhkennedy
Copy link
Collaborator Author

@pbrotoisworo you're correct, the current conda-forge PR (Supporting MintPy 1.3.1) will not have windows builds for PySolid and MintPy.

Once all three recipes get on conda-forge, they will have their own feedstocks and it'll be easier and faster to fix the PySolid recipe (I won't have to wait on the other builds and dig through all the logs for all three recipes). It shouldn't be too hard I just have to fight through some linking issues. So shortly there-after there'll be a new build with windows support -- I'll ping you when it's available.

@yunjunz
Copy link
Member

yunjunz commented Sep 6, 2021

Hi @jhkennedy, I just tried conda install mintpy in the hyp3 channel as you noted above. Never had I installed mintpy so simple and fast!! Testing based on the Fernandina example dataset passed, which is great!

Here are my feedbacks/suggestions:

  1. The $MINTPY_HOME env var missed the first half of the full path, as shown below; while the correct should be like ~/tools/miniconda3/envs/mintpy/lib/python3.8/site-packages/mintpy
(mintpy) kamb:~>$ echo $MINTPY_HOME
/lib/python3.8/site-packages/mintpy
  1. smallbaselineApp.py, view.py and tsview.py are copied over to the envs/mintpy/bin folder. I know that this is how the setup.py does (which is not fully functioning BTW), but this is not the preferred way for the current structure of mintpy. The preferred way would be adding $MINTPY_HOME to the $PATH without copying over anything to the bin folder, so that all scripts under it are available in the command line. This would be similar to the $ISCE_HOME in the isce-2 conda recipe.

  2. Could we add the test directory to lib/python3.8/site-packages/mintpy? Maybe I should rename it to tests following the convention of other software?

  3. Similar to 3, but for pysolid.

@jhkennedy
Copy link
Collaborator Author

  1. The $MINTPY_HOME env var missed the first half of the full path, as shown below; while the correct should be like ~/tools/miniconda3/envs/mintpy/lib/python3.8/site-packages/mintpy

Yep! I fixed it in this commit for the staged recipe (I did some simplifying/refactoring and forgot to switch PREFIX, which is used during build, to CONDA_PREFIX which is an env variable available for for the activation/deactivation scripts).

  1. smallbaselineApp.py, view.py and tsview.py are copied over to the envs/mintpy/bin folder. I know that this is how the setup.py does (which is not fully functioning BTW), but this is not the preferred way for the current structure of mintpy. The preferred way would be adding $MINTPY_HOME to the $PATH without copying over anything to the bin folder, so that all scripts under it are available in the command line. This would be similar to the $ISCE_HOME in the isce-2 conda recipe.

That'll need to be fixed in MintPy itself and is associated with the entrypoints part of #652 (I've expanded there).

  1. Could we add the test directory to lib/python3.8/site-packages/mintpy?

We could, but with the current test setup, I don't think it's a good idea. The current tests are more integration tests (download data, long run times, etc) which are likely to overtax the limited CI/CD infrastructure conda-forge provides (for free!). Generally, conda-forge test are to check basic things -- is the package installed correctly (files are where I expect them, I can execute the expected scritps, etc), not is the installed package running correctly. Integration tests are better handled before release by the package itself.

Note: This is a science package centric view, where integration and larger testing usually involves "large" data sets and longer run times. There are ways to package tests with the package itself, but those tests should be small and fast.

  1. ... Maybe I should rename it to tests following the convention of other software?

I think this is probably a good idea if you're willing to! Most all the python testing software (e.g., pytest) will use tests as the directory name. that said, there's nothing wrong with test (well, in some circumstances it could conflict with the builtin test, but that's not a huge worry), and most everyone will understand what the test directory is for.

  1. Similar to 3, but for pysolid.

For PySolid, we'll definitely want to test whether the compiled code is compiled correctly and some basic unit tests would be nice. I haven't looked at those tests much yet, so not sure, but the same generally applies for MintPy and PySolid.

@jhkennedy
Copy link
Collaborator Author

🥳 conda-forge/staged-recipes#16050 was merged! I'll close this issue (if no objections) once the feedstock is created and build is available.

@yunjunz
Copy link
Member

yunjunz commented Sep 9, 2021

Thank you @jhkennedy for the awesome contribution!!! I cannot wait to try it.

@jhkennedy
Copy link
Collaborator Author

jhkennedy commented Sep 10, 2021

PyAPS3 and PySolid are all now available for install on conda-forge!

PyAPS3 0.2.0 (noarch; linux, mac, windows for Python >=3.6)

conda install -c conda-forge pyaps3

Recipe will be maintained in the conda-forge/pyaps3-feedstock.

PySolid v0.1.2 (linux, mac for Python >= 3.6)

conda install -c conda-forge pysolid

Recipe will be maintained in the conda-forge/pysolid-feedstock.


Note: Still waiting for MintPy to be available

@jhkennedy
Copy link
Collaborator Author

MintPy is available for install on conda-forge!

MintPy v1.3.1 (linux, mac for Python >=3.6,<3.9)

conda install -c conda-forge mintpy

Recipe will be maintained in the conda-forge/mintpy-feedstock.

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

No branches or pull requests

5 participants