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

Better handle package data for conda build #467

Merged
merged 7 commits into from
Jan 18, 2023

Conversation

jhkennedy
Copy link
Collaborator

@jhkennedy jhkennedy commented Jan 11, 2023

The current conda-forge build is missing a critical "package data" file. This PR explicitly calls out *.yml and *.yaml files inside src/RAiDER as package data so they are included in the archive.

NOTE: while a MANIFEST.in works for pip and setuptools everywhere outside of conda, conda-build would still not include the package data in the conda build. This is the only method that I could get to work.


Explicitely calling out the package data, or including a MANIFEST.in, isn't supposed to be needed because everything is included via setuptools_scm. Importantly, when building the RAiDER distribution setuptools_scm uses the git history (tags, tracked files) to define the package version and included files and produces a built src or binary distribution, which pip uses to install the package.

when building the conda package, we're not actually cloning the repo, but instead downloading a git archive with does not include the git history. For the package version, we explicitly tell setuptools_scm which version to use:
https://github.com/conda-forge/raider-feedstock/blob/main/recipe/meta.yaml#L14
But we also need to explicitly tell it what files to include, which is what this PR does.


Alternatively we could switch to building the conda package from distribution published to PyPI, which will include metadata describing the version package files to include, but pip installs from PyPI won't work (easily at least) as we have dependencies that are conda-forge only (e.g., ISCE3) and compiled extensions (we'd need to build binary distributions for a variety of architectures). This is not a good user experience so we shouldn't do this unless we do the work to make installing from PyPI relatively easy.

@jhkennedy jhkennedy added the bug Something isn't working label Jan 11, 2023
@jlmaurer
Copy link
Collaborator

jlmaurer commented Jan 11, 2023

@jhkennedy @dbekaert this seems fine to me. Will this manifest catch all package files? I didn't see a "*.yaml" or anything like so wasn't sure.

This is not a good user experience so we shouldn't do this unless we do the work to make installing from PyPI relatively easy

It is a bit annoying that it would be so much work to make a PyPI package; probably I would vote against it for the moment, seems like a lot of work to be added when we are trying to get up to a stable version.

@jhkennedy
Copy link
Collaborator Author

Will this manifest catch all package files? I didn't see a "*.yaml" or anything like so wasn't sure.

@jlmaurer I'm still working out if this actually fixes it or not, but yes, the intent is to include all of it, which is what the graft keyword is supposed to do: https://packaging.python.org/en/latest/guides/using-manifest-in/

@jhkennedy
Copy link
Collaborator Author

jhkennedy commented Jan 11, 2023

To check the conda package locally, I'm doing:

git clone -b conda-manifest git@github.com:jhkennedy/raider-feedstock.git
cd raider-feedstock

./build_locally.py linux_64_numpy1.21python3.10.____cpython

cd build_artifacts/linux-64
unzip raider-0.4.0-py310h9b08913_1.conda
tar --use-compress-program=unzstd -tf pkg-raider-0.4.0-py310h9b08913_1.tar.zst | sort

Note: I'm applying this change as a patch because I'm attempting to patch the current release on conda-forge as well; see conda-forge/raider-feedstock#7

This unfortunately still doesn't show a lib/python3.10/site-packages/RAiDER/cli/raider.yaml file being added to the package 😭

@jhkennedy jhkennedy changed the title Add a maniftest.in for conda build Better handle package data for conda build Jan 18, 2023
@jhkennedy
Copy link
Collaborator Author

Okay, after entirely too many iterations, I've got a setup that works for:

  • pip installed (editable and not)
  • sdist and .whl builds of the package
  • conda-build tested locally.

So the raider.yaml package data file should now be included everywhere.

@jhkennedy jhkennedy marked this pull request as ready for review January 18, 2023 06:41
@jhkennedy
Copy link
Collaborator Author

The miniconda GitHub action has been having a ton of intermittent environment creation errors, causing a lot of the checks to fail. I've also:

  • dropped minconda for micromamba
  • bumped the reusable actions versions

which should resolve those issues. More discussion here: ASFHyP3/actions#49

@jlmaurer
Copy link
Collaborator

@jhkennedy that's some great work!

One of our unit tests is pinging a server that is evidently down a lot, which is why circleCI is failing. pinging @dbekaert and @bbuzz31 just to let them know. The problem test is in test_datelist and I believe calls the GMAO server. We don't need to worry about it for this PR.

@jhkennedy jhkennedy merged commit bf6b545 into dbekaert:dev Jan 18, 2023
@jhkennedy jhkennedy deleted the conda-manifest branch January 18, 2023 16:22
@jhkennedy jhkennedy mentioned this pull request Jan 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants