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

Build for pypi instead of conda #131

Merged
merged 35 commits into from
Feb 22, 2022
Merged

Build for pypi instead of conda #131

merged 35 commits into from
Feb 22, 2022

Conversation

vladsavelyev
Copy link

@vladsavelyev vladsavelyev commented Feb 9, 2022

Simplify maintaining the conda package recipe: instead of manually changing the list of dependencies, populate them automatically from the pip requirements list.

Build cpg-hail pip package instead of conda.

@vladsavelyev vladsavelyev changed the title Conda env: more missing packages and changed version pins Conda recipe: render from pip requirements Feb 10, 2022
@vladsavelyev vladsavelyev marked this pull request as ready for review February 10, 2022 10:39
Copy link

@illusional illusional left a comment

Choose a reason for hiding this comment

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

The first half is great, I think templating the conda recipe is fine, but I'm not super comfortable with the script as it stands. The translation of pypi packages to conda packages feels risky, I think it'd be better to completely switch everything over to conda if possible, or I'd feel better about maintaining a json map or something.

I'd prefer a templating engine, or if not I think there should be tests for the templating. Build up the string completely, then write it. There's not much memory benefit doing it line by line.

conda/render_recipe.py Outdated Show resolved Hide resolved
conda/render_recipe.py Outdated Show resolved Hide resolved
conda/render_recipe.py Outdated Show resolved Hide resolved
conda/render_recipe.py Outdated Show resolved Hide resolved
conda/render_recipe.py Outdated Show resolved Hide resolved
@vladsavelyev
Copy link
Author

I didn't intend to make the render_recipe.py script super reliable. Basically it's just intended to save us from manually updating the conda recipe deps. When making this PR, I first just extended the already existing Fix meta YAML CI section by adding a bunch of seds into the run: command, but then I realized it would be cleaner done in python, and finally python code got bigger and I extracted it into a separate script.

But I'm happy to keep render_recipe.py locally on my laptop, and push an already rendered recipe whenever we merge from upstream.

I think it'd be better to completely switch everything over to conda if possible, or I'd feel better about maintaining a json map or something.

The Hail team maintains a pip package, so it would make sense to switch to pip entirely. One downside is that pip doesn't have channels, so we would have to prefix the package name, e.g. cpg-hail to avoid clashes with the official package.

Ideally we don't want to diverge from the official release and have two packages. But not sure if that's possible. Currently there are a couple of dataproc changes (which will become irrelevant soon), and passing token to the backend (which we might try to upstream?), but even if these go away, we might need to diverge somewhere else.

@vladsavelyev vladsavelyev changed the title Conda recipe: render from pip requirements Build for pypi instead of conda Feb 16, 2022
@vladsavelyev
Copy link
Author

Modified the workflow to build cpg-hail package instead of a conda one.

.github/workflows/build_package.yaml Outdated Show resolved Hide resolved
.github/workflows/build_package.yaml Outdated Show resolved Hide resolved
@vladsavelyev vladsavelyev merged commit 4d16aa7 into main Feb 22, 2022
@vladsavelyev vladsavelyev deleted the fix-conda2 branch February 22, 2022 00:59
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.

2 participants