-
Notifications
You must be signed in to change notification settings - Fork 87
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 conda on merge to main #1247
Conversation
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.
@ryan-s this is awesome! I left some comments to explain to the team what's going on (feel free to correct me where I'm mistaken hehe).
We ran this check on circle ci with main and a purposely broken branch and the check is working as expected!
cp -r `ls -A | grep -v "evalml-core-feedstock"` ./evalml-core-feedstock/evalml/ | ||
python conda_config.py "$(python setup.py --version)" | ||
cd evalml-core-feedstock | ||
echo "$DOCKER_HUB_PASS" | docker login -u psalter --password-stdin |
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.
We have manually check out this container and run docker run
ourselves as opposed to running run_docker_build.sh
in the feedstock repo because circle-ci doesn't let you mount volumes to the docker image. I think this is ok because we're not straying far from the contents of that script. @ryan-s and I discussed ways of tightening this up in the future too!
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.
@ryan-s @freddyaboulton is this env var DOCKER_HUB_PASS
set by circleci? Did you have to enter it?
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.
@dsherry I entered it in circleci. Not sure if we have a vault somewhere to share the credentials?
- shellcheck/check | ||
filters: | ||
branches: | ||
only: main |
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 means that this will run after a PR gets merged to main
.
We created this check to catch breaking changes to our conda recipe (like new dependencies or dependency updates) before release day and having it run after a merge accomplishes that without having to slow down development or consume too many ci resources.
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.
very cool!
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.
SGTM!
Codecov Report
@@ Coverage Diff @@
## main #1247 +/- ##
=======================================
Coverage 99.93% 99.93%
=======================================
Files 207 207
Lines 13031 13031
=======================================
Hits 13022 13022
Misses 9 9 Continue to review full report at Codecov.
|
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.
@ryan-s this rocks!!! I'm so excited you're adding this! It's gonna make our release process a lot simpler and less likely to break conda. Woohoo!!
So, I take it we'll still need to do the actual conda release manually, right?
@freddyaboulton once this is in, could you update release.md to reflect this?
@ryan-s I left a bunch of questions, and two comments to address before merge:
- Please move that new file
conda_config.py
to somewhere other than the top-level directory -- I think it should go in.circleci/
. Hmm, on reflection, since its python code, perhaps it should go in a new folderevalml/tests/circleci/
. Your call there. - I left a comment about using
Pathlib
--let's resolve that, either by making the change or by you telling me its unnecessary lol
🚢
- shellcheck/check | ||
filters: | ||
branches: | ||
only: main |
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.
SGTM!
cp -r `ls -A | grep -v "evalml-core-feedstock"` ./evalml-core-feedstock/evalml/ | ||
python conda_config.py "$(python setup.py --version)" | ||
cd evalml-core-feedstock | ||
echo "$DOCKER_HUB_PASS" | docker login -u psalter --password-stdin |
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.
@ryan-s @freddyaboulton is this env var DOCKER_HUB_PASS
set by circleci? Did you have to enter it?
export UPLOAD_PACKAGES=False | ||
export HOST_USER_ID=$(id -u) | ||
export FEEDSTOCK_NAME=evalml-core-feedstock | ||
docker run -t -e CONFIG -e HOST_USER_ID -e UPLOAD_PACKAGES -e GIT_BRANCH -e UPLOAD_ON_BRANCH -e CI -e FEEDSTOCK_NAME -e CPU_COUNT -e BINSTAR_TOKEN -e FEEDSTOCK_TOKEN -e STAGING_BINSTAR_TOKEN psalter/build:latest bash /home/conda/feedstock_root/.scripts/build_steps.sh |
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.
If I'm following right, this command will have a non-zero return code if there's an error? Meaning the CI job will get flagged as failed?
Will the error output appear in the CI job, or do we have to do something else to get the error output? It would be amazing to have the output appear in the CI job, because otherwise its gonna be hard to debug.
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 error appears in the CI job!
- checkout | ||
- install_dependencies_test | ||
- setup_remote_docker: | ||
version: 19.03.12 |
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 this version of? Is this the version of docker itself? And is setup_remote_docker
a circleci predefined macro, or something else?
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.
Correct, this is the latest version of docker that circleci supports and it is predefined by them.
working_directory: ~/evalml/ | ||
executor: | ||
name: python | ||
python_version: "3.6" |
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.
@ryan-s @freddyaboulton why python 3.6? Should we do this for all our supported python versions, 3.6, 3.7 and 3.8? Or just the latest version, 3.8?
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 sets the version of python in the circleci worker. The docker run command will actually build the package in python 3.7.8 - this is the same as what happens in the CI of our feedstock repo. Unfortunately, conda doesn't provide an out-of-the-box way to build with different python versions but I will add this to the future work section of our design doc for this issue.
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.
Ah got it. So we'll only build and run our tests on 3.7.8 for now. No problem, this is a great starting point. Thanks!
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.
Correct, I just needed something to install pyyaml into so that we can update the recipe.
I think the v2 of this would be to create our own container(s) based off of condaforge/linux-anvil-comp7 where we can move the existing functionality and any new improvements into.
conda_config.py
Outdated
parser = argparse.ArgumentParser(description="Configure conda for local build. Run from the feedstock root") | ||
parser.add_argument('version', help='The version of EvalML being built') | ||
args = parser.parse_args() | ||
write_conda_recipe(args.version) |
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.
@ryan-s this is super cool!!
Please move this file to a different location instead of the top-level directory. Should it go in .circleci/
? That would be my first inclination.
conda_config.py
Outdated
version: The version of EvalML we are building with this feedstock | ||
|
||
Returns: | ||
None: Side effect of overwriting the existing meta.yaml in the feedstock |
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.
Amazing, thank you for documenting.
conda_config.py
Outdated
class CondaDumper(yaml.Dumper): | ||
|
||
def increase_indent(self, flow=False, indentless=False): | ||
return super(CondaDumper, self).increase_indent(flow, False) |
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.
Why is this necessary? Is this just a style thing, or is it a functional thing?
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.
Unfortunately it is functional. The existing recipe is actually a Jinja2 template masquerading as a yaml file. Whatever they are using to parse that template requires that we have the increased indents even though the YAML spec does not.
@@ -7,3 +7,4 @@ codecov==2.1.0 | |||
category_encoders>=2.0.0 | |||
featuretools | |||
nlp_primitives>=1.0.0 | |||
PyYAML==5.3.1 |
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.
👍
conda_config.py
Outdated
# Toss out the first line that declares the version since its not supported YAML syntax | ||
next(config_file) | ||
config = yaml.safe_load(config_file) | ||
recipe_path = f'../feedstock_root/evalml' |
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.
@ryan-s could you please use pathlib.Path
here instead? For hypothetical windows support haha
recipe_path = str(pathlib.Path('..', 'feedstock_root', 'evalml'))
Also what is this path? Like, this is intended to be the path on the docker container? A comment explaining this would be super helpful to future readers.
Yes, we still need to manually edit the recipe file in the feedstock if there are dependency changes in a release but now we can skip the "Test conda version before releasing on PyPI" section of the release process! I'll update once this is in. |
As of right now yes, but in theory it wouldn't be too hard to copy the artifact out of the container and push it. You would just need to do the inverse of what we do when we copy our feedstock into the container. |
Pull Request Description
Add in a conda package build as part of the ci when a PR is merged into main. This PR adds in a small python script to change the recipe to use the local build (conda_config.py). Once we have that, we clone the feedstock and start to replicate what the python and shell scripts were doing do to issues on circleCI. The end result is a build that passes or fails and produces no artifacts.
After creating the pull request: in order to pass the release_notes_updated check you will need to update the "Future Release" section of
docs/source/release_notes.rst
to include this pull request by adding :pr:123
.