Write a "dependency update warning" PR checkin test #324

dsherry · 2020-01-31T15:29:30Z

Steps:

Save the output of pip freeze in repo
PR checkin test which does the following:
- Run pip install
  *Run pip freeze and diff the output with saved version
- If there's no diff, mark as green
- If there's a diff, mark as failed and print out the diff

When the PR checkin test fails, that means a dependency has changed since the last PR was merged to master. The PR author will update evalml/tests/latest_dependency_versions.txt and merge

After creating the pull request: in order to pass the changelog_updated check you will need to update the "Future Release" section of docs/source/changelog.rst to include this pull request by adding :pr:123.

dsherry · 2020-01-31T15:31:26Z

@rwedge @angela97lin so RE our discussion yesterday, is there any way to configure this new test to only run as a PR checkin test? That seems like the right thing to do

rwedge · 2020-01-31T15:32:24Z

As in not on master?

angela97lin · 2020-01-31T15:34:09Z

Do you mean not running on the master branch? Like:

          filters:
            branches:
              ignore: /^master?/

dsherry · 2020-01-31T15:36:32Z

@rwedge yeah. But hmm, I guess there's a few options here. It's fine if this runs on master. But in that case, how can we configure notifications for if it fails?

Question: if this job is failing, should it block any PR merges until it succeeds? I can see both sides of this. On the one hand, it would be a clear signal to the devs that something is wrong and needs to be fixed ASAP, i.e. a new PR needs to go up to update the dependencies txt file and ensure all tests are green before merging. On the other hand, blocking dev is not great, and if this just ran on master and made a lot of noise when it failed, that would be a win too. @rwedge do you have an opinion?

dsherry · 2020-01-31T15:40:19Z

@angela97lin yeah. But it's my understanding that using the syntax you provided means this would still run in the background on any commit to any other branch, not just as a PR checkin test. Is that right? That seems like remarkable overkill 😂and is what I'd like to avoid.

There's a few options which come to mind:

Have this test run on changes to master
Have this test run as part of the checkin tests, blocking development until it passes
Both of the above (which feels like overkill)
Is there a way to have the checkin test marked yellow rather than red, which would allow merges but still warn devs there's an issue? If so, we could do that

angela97lin · 2020-01-31T15:45:38Z

Ah, I see. Hmmm... Correct me if I'm wrong: based on what we're trying to accomplish here, each time a package is updated, we'll fail the test until we install the newest version? Will we always want to upgrade our requirements to the latest and greatest?

4. Is there a way to have the checkin test marked yellow rather than red, which would allow merges but still warn devs there's an issue? If so, we could do that

It's okay if it's marked red to indicate that there's an issue since devs (admins, which I think we all are) can still choose to merge even if all checks don't pass :P

rwedge · 2020-01-31T15:45:55Z

You can use the branch settings of the github repo to specify this check is not required to merge in

rwedge · 2020-01-31T15:51:39Z

Are devs supposed to maintain this dependency text file and commit it along with their PR? What if their dev environment has extra packages installed? Can we automate generating this file?

angela97lin · 2020-01-31T16:00:23Z

That was also my concern with the pip freeze approach. Not sure if this would be nearly as comprehensive as pip freeze but I know FT has an info CLI which lists dependencies and their versions. I'm currently in the middle of implementing the same for evalml (#255). Perhaps we could use that instead? :)

rwedge · 2020-01-31T16:13:03Z

Could we run pip freeze or evalml info only in the automated tests and store the results on S3? Then we would know it was a clean environment each time. Diff the results of a current checkin test with the results of a previous run of this PR / merge to master?

codecov · 2020-01-31T16:25:33Z

Codecov Report

Merging #324 into master will not change coverage by %.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #324   +/-   ##
=======================================
  Coverage   97.36%   97.36%           
=======================================
  Files         104      104           
  Lines        3266     3266           
=======================================
  Hits         3180     3180           
  Misses         86       86

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 910b5c1...73be385. Read the comment docs.

dsherry · 2020-01-31T19:15:49Z

Are devs supposed to maintain this dependency text file and commit it along with their PR?

@rwedge yeah I have the job saving the new dependencies as a txt artifact. I like the idea of these versions being in the repo--it's a version-controlled, living document of our current latest supported dependency versions.

What if their dev environment has extra packages installed?

That should be fine. This check isn't running in their dev env. If the job fails, it means something has changed in the dependency versions available on the CircleCI linux environment.

When the check fails in a PR, that means its currently also failing on master. That would indicate we need to get a separate PR up ASAP to update the dependency version txt file in the repo. The if we encounter problems in that new PR, we can file an issue and fix the version to prevent the new version from being used.

Can we automate generating this file?

This PR does that already! Its a build artifact. I think the next step would be to somehow automate updating the file. That would be helpful, but not required today IMO.

Could we run pip freeze or evalml info only in the automated tests and store the results on S3? Then we would know it was a clean environment each time. Diff the results of a current checkin test with the results of a previous run of this PR / merge to master?

I'm not sure how that suggestion is different from what my PR does right now, except that I'm storing the versions in the repo right now instead of S3. If I'm missing something, lmk--I didn't understand the last sentence there.

Not sure if this would be nearly as comprehensive as pip freeze but I know FT has an info CLI which lists dependencies and their versions. I'm currently in the middle of implementing the same for evalml (#255). Perhaps we could use that instead? :)

@angela97lin yeah that will definitely be helpful to have! Why not both :)

rwedge · 2020-01-31T20:07:18Z

@dsherry, so is this the plan, basically?

There is a txt file in the github repo that tracks the versions of dependencies used when running EvalML check-in tests
Each time the tests are run, a file is created with the dependencies used on that run. This file is compared with the one in the repo. If there are differences, the check fails. The differences are saved to a text file on Circle CI. The file in github is not modified.
If this check fails, we can create a separate PR that updates the dependencies file in the repo to match the new "latest" versions, and handles fixing any bugs encountered solely due to this update.

dsherry · 2020-01-31T20:23:35Z

@rwedge yep, that sounds good to me. And is what this PR should be doing currently. I think all that's missing is to configure this as a checkin test, rather than running on master

dsherry · 2020-01-31T20:37:19Z

I think this is working properly. Once I fixed the config schema, the job added itself to my PR. That's super cool.

It failed the first time, because the dependencies I committed were from my mac. Here's the failed job. It contains two artifacts: the pip freeze diff and the full list of latest pip-installed dependency versions.

So @rwedge @angela97lin from my perspective this is ready to go. Do you agree?

Is there a better name for the workflow than dependencies_updated_linux? Perhaps check_dependencies_updated_linux

dsherry · 2020-01-31T20:40:21Z

.circleci/config.yml

+            python_version: "3.7"
+            filters:
+              branches:
+                ignore: /^master?/


@rwedge @angela97lin does this look like the correct config to have this job only run as a checkin test?

@dsherry this won't run on master but if Circle CI is set to run on every commit, not just PRs, it will still run on every commit for branches that aren't master

dsherry · 2020-02-07T01:41:34Z

I see that all jobs except for the new job are marked as "Required" in the github UI. I'm guessing that means I'm missing a flag somewhere in the config?

dsherry · 2020-02-07T01:45:24Z

Aha, I see I need to add it as a branch rule in the github settings. I will do so once this PR has been merged.

dsherry · 2020-02-12T17:50:06Z

Ping @rwedge @angela97lin @christopherbunn can one of you please review this when you have
a chance?

.circleci/config.yml

dsherry · 2020-02-12T21:40:14Z

Shoot. The job is failing because of the evalml dependency:

#!/bin/bash -eo pipefail
echo Displaying dependencies which have changed, with master on the left and the new branch on the right:
cat /tmp/dependencies_updated_artifacts/diff.txt
Displaying dependencies which have changed, with master on the left and the new branch on the right:
27c27
< -e git+git@github.com:FeatureLabs/evalml.git@2a4e126645ee72133b1c372f5d8ed8a65631ec37#egg=evalml
---
> -e git+git@github.com:FeatureLabs/evalml.git@0c21e3207c7e225ddbb963d520f8572a87d77450#egg=evalml
CircleCI received exit code 0

I'll add code to ignore evalml in the diff.

dsherry · 2020-02-14T16:03:37Z

Hey @rwedge @angela97lin @christopherbunn this PR is ready to go, I just need an approval :)

dsherry · 2020-02-14T16:08:46Z

It's working!! The job just failed on this PR:

Displaying dependencies which have changed, with master on the left and the new branch on the right:
11a12
> catboost==0.21

If this PR were merged when @angela97lin had her catboost PR up, the checkin test would have failed on her PR with this message, and she'd have to update evalml/tests/latest_dependency_versions.txt to include catboost before merging.

Fixing now

rwedge · 2020-02-14T17:29:32Z

I think the code looks good. The python 3.5 tests are failing for some reason though

christopherbunn · 2020-02-14T17:37:23Z

Re: the failing python 3.5 test, it's related to #167. I think you can go into CircleCI, hit "rerun from failed workflow", and it'll do the python 3.5 test again. More likely than not it should pass the test the second time around.

…eps changing

dsherry · 2020-02-14T18:29:25Z

Yeah @rwedge as @christopherbunn said, the linux python 3.5 tests have been flaky for a while. Tracked by #167 . It's annoying; we'll get it fixed.

Thanks for reviewing. You wanna give this pr one of those big green checkmarks? :)

dsherry added the testing Issues related to testing. label Jan 31, 2020

dsherry requested review from rwedge and angela97lin January 31, 2020 15:29

dsherry self-assigned this Jan 31, 2020

dsherry force-pushed the ds_323_dependency_update_warning branch from b136f34 to edc6d0a Compare January 31, 2020 16:20

dsherry force-pushed the ds_323_dependency_update_warning branch from edc6d0a to 9a690f4 Compare January 31, 2020 20:24

dsherry commented Jan 31, 2020

View reviewed changes

dsherry force-pushed the ds_323_dependency_update_warning branch from 340129c to c836eda Compare January 31, 2020 22:20

dsherry requested a review from christopherbunn February 11, 2020 19:03

rwedge reviewed Feb 12, 2020

View reviewed changes

.circleci/config.yml Show resolved Hide resolved

dsherry force-pushed the ds_323_dependency_update_warning branch from c836eda to 2a4e126 Compare February 12, 2020 21:30

dsherry requested a review from rwedge February 13, 2020 13:58

dsherry force-pushed the ds_323_dependency_update_warning branch from 68298df to 4f7636f Compare February 14, 2020 16:02

dsherry added 16 commits February 14, 2020 13:26

Add output of pip freeze.

f267cad

Add circleci config

5a68ba6

Fixed path error. Add on_fail to print out diff

5a3157e

lint

d41a8c6

Lint

77e3ad6

Lint

73cbdf1

Changelog

21893d0

Add job to test_all_python_versions target

aa7f5af

Move to separate workflow

5e0df02

Lint

791404a

Use the dependency versions from circleci linux

cd6c13e

Changed job name/descr

120ddd6

Activate virtualenv

e6f4ae3

Update dependencies to correspond to virtualenv

1264ad8

Don't include evalml as a dependency in the diff, because the hash ke…

ae91f8b

…eps changing

Add catboost to expected deps

73be385

dsherry force-pushed the ds_323_dependency_update_warning branch from f191890 to 73be385 Compare February 14, 2020 18:29

rwedge approved these changes Feb 14, 2020

View reviewed changes

dsherry merged commit 41292d9 into master Feb 14, 2020

dsherry deleted the ds_323_dependency_update_warning branch February 14, 2020 21:04

dsherry mentioned this pull request Feb 14, 2020

Write a "dependency update warning" PR checkin test #323

Closed

angela97lin mentioned this pull request Mar 9, 2020

v0.7.0 #471

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Write a "dependency update warning" PR checkin test #324

Write a "dependency update warning" PR checkin test #324

dsherry commented Jan 31, 2020

dsherry commented Jan 31, 2020

rwedge commented Jan 31, 2020

angela97lin commented Jan 31, 2020

dsherry commented Jan 31, 2020

dsherry commented Jan 31, 2020

angela97lin commented Jan 31, 2020

rwedge commented Jan 31, 2020

rwedge commented Jan 31, 2020

angela97lin commented Jan 31, 2020

rwedge commented Jan 31, 2020

codecov bot commented Jan 31, 2020 •

edited

Loading

dsherry commented Jan 31, 2020

rwedge commented Jan 31, 2020 •

edited

Loading

dsherry commented Jan 31, 2020

dsherry commented Jan 31, 2020 •

edited

Loading

dsherry Jan 31, 2020

rwedge Jan 31, 2020 •

edited

Loading

dsherry commented Feb 7, 2020

dsherry commented Feb 7, 2020

dsherry commented Feb 12, 2020

dsherry commented Feb 12, 2020

dsherry commented Feb 14, 2020

dsherry commented Feb 14, 2020

rwedge commented Feb 14, 2020

christopherbunn commented Feb 14, 2020

dsherry commented Feb 14, 2020

Write a "dependency update warning" PR checkin test #324

Write a "dependency update warning" PR checkin test #324

Conversation

dsherry commented Jan 31, 2020

dsherry commented Jan 31, 2020

rwedge commented Jan 31, 2020

angela97lin commented Jan 31, 2020

dsherry commented Jan 31, 2020

dsherry commented Jan 31, 2020

angela97lin commented Jan 31, 2020

rwedge commented Jan 31, 2020

rwedge commented Jan 31, 2020

angela97lin commented Jan 31, 2020

rwedge commented Jan 31, 2020

codecov bot commented Jan 31, 2020 • edited Loading

Codecov Report

dsherry commented Jan 31, 2020

rwedge commented Jan 31, 2020 • edited Loading

dsherry commented Jan 31, 2020

dsherry commented Jan 31, 2020 • edited Loading

dsherry Jan 31, 2020

Choose a reason for hiding this comment

rwedge Jan 31, 2020 • edited Loading

Choose a reason for hiding this comment

dsherry commented Feb 7, 2020

dsherry commented Feb 7, 2020

dsherry commented Feb 12, 2020

dsherry commented Feb 12, 2020

dsherry commented Feb 14, 2020

dsherry commented Feb 14, 2020

rwedge commented Feb 14, 2020

christopherbunn commented Feb 14, 2020

dsherry commented Feb 14, 2020

codecov bot commented Jan 31, 2020 •

edited

Loading

rwedge commented Jan 31, 2020 •

edited

Loading

dsherry commented Jan 31, 2020 •

edited

Loading

rwedge Jan 31, 2020 •

edited

Loading