-
Notifications
You must be signed in to change notification settings - Fork 3
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
Feature/circleci fv3net build push #192
Conversation
Sorry about all the commits! Quite a bit of trial and error involved with CircleCI on this. Edit: all seems to be good on the home front |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The bumpversion configuration looks fine to me.
commit = True | ||
tag = False | ||
|
||
[bumpversion:file:setup.py] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks Like you got all the ones I added.
search = {current_version} | ||
replace = {new_version} | ||
|
||
[bumpversion:file:Makefile] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The wheels built by the makefile have version strings, but I stopped using the wheels in #193 anyway, so let's not worry about.
Co-authored-by: W. Andre Perkins <frodre@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for putting this together @frodre. I think this will be a big improvement for all of our workflows. I do have some concerns that the changes to the dependency management system will make it harder to debug and find errors due to upstream version changes. Perhaps we should talk on zoom.
command: cat environment.yml .circleci/.installed_build_env_deps > combined_deps.txt | ||
- restore_cache: | ||
keys: | ||
- v2-fv3net-env-{{ checksum "combined_deps.txt" }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is pretty cool. But I worry that this will make it tricky to see if we get regressions when the resolved dependencies in environment.yml
change. For instance, when working on #185 the same environment.yml
started installing pandas 2.0, which broke xarray. With this caching strategy we would not catch such errors, so the CI might still pass even though people's local environments and manually pushed images fail.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I had similar concerns. I feel like the answer could be having some type of more rigorous detailing of versions to install than just the environment.yml
. I tried caching the conda pkgs directory and pip directory, but that wasn't any faster than the old mechanism. Perhaps the builds would depend on some usage of the conda list --explicit
file and whatever we need to add the pip
packages back in could work.
jobs: | ||
build: | ||
pytest_default: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems somewhat redundant to build both the docker image with the environment, and the environment here. Maybe we should run these tests within the docker images built below. I think that is what we do the Circle CI fv3gfs builds.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The pytest_default
runs as our normal testing framework for branches. Build only calls on merge to master and releases. I think we want to leave the pytest step rather than building images on each branch? We can clarify this in chat.
use_version=$CIRCLE_TAG | ||
fi | ||
make build_images VERSION=$use_version | ||
- run: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider moving the tests here, and deleting the pytest_default
job.
Makefile
Outdated
wheels: | ||
pip wheel --no-deps . | ||
pip wheel --no-deps external/vcm | ||
# wheels: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove this commented code (I know I did this).
Release naming guidelines | ||
------------------------- | ||
|
||
If `x.y.z` is the version, bump `y` (minor) on new features or breaking changes, and `z` on smaller changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this convention should only hold while we are 0.y.z
. Once we reach 1.0, we should start using semver, which means we bump x
for breaking versions, y
for new features, and z
for patches.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, totally makes sense. I used this format for now more for the explanation below to use 'x' rather than '0' for the usage description of bumpversion
docker/prognostic_run/Dockerfile
Outdated
COPY fv3net-0.1.0-py3-none-any.whl /wheels/fv3net-0.1.0-py3-none-any.whl | ||
COPY vcm-0.1.0-py3-none-any.whl /wheels/vcm-0.1.0-py3-none-any.whl | ||
RUN pip3 install --no-deps /wheels/fv3net-0.1.0-py3-none-any.whl && pip3 install /wheels/vcm-0.1.0-py3-none-any.whl | ||
RUN pip3 install wheel |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this line can be removed actually.
- pip | ||
- h5netcdf>=0.8 | ||
- h5py>=2.10 | ||
- netcdf4>=1.4 | ||
- pip: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the motivation for moving things to pip? I realize it is faster, but conda is more robust and actually does proper dependency resolution. OTOH, the pip install will change based on the order of the packages here, with the final options taking precedence. For instance, pip install numpy==1.16 pandas==1.17
does not guarantee that the installed numpy verison is 1.16
if pandas requires a higher version. In this case, conda
will fail with an dependency resolution behavior. I think this change could make debugging and fixing such errors a lot harder, especially since so few of these packages actually have constrained versions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I reset it to the old style where most dependencies are resolved through conda.
Ready for re-review @nbren12. Adjustments:
Thanks for the comments/suggestions! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great! LGTM @frodre.
This PR adds automated version bumping and CircleCI integration for building/pushing images related to
fv3net
.Changes:
master
is updatedfv3net
dependencies of the conda environment for the pytest_default job. This significantly shortens the testing process.build_environment.sh
to additionally output a list of all installed packages under.circleci/.installed_build_env_deps
. This is used for caching python dependencies on CircleCI and for reproducibility down the line.environment.yml
to install fewer packages using conda and more using pip. This also shortens the dependency install time by reducing the conda dependency graph search.