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

Automate latest dependency PR #482

Merged
merged 30 commits into from Mar 13, 2020
Merged

Automate latest dependency PR #482

merged 30 commits into from Mar 13, 2020

Conversation

jeremyliweishih
Copy link
Contributor

@jeremyliweishih jeremyliweishih commented Mar 11, 2020

Fixes #467.

This works through a github action on a cron schedule for everyday at 8:30 AM. I forked the create-pull-request github action (now under Featurelabs/create-pull-request) created by Peter Evans so we can review and keep the action static for security purposes.

Example of PR (changes not shown here): #480

The action works by checking if there is a diff between the base branch (when merged the base branch will be master) and the temp branch in the action runner. If there is a PR will be created and won't if not. More info here.

@jeremyliweishih jeremyliweishih self-assigned this Mar 11, 2020
@codecov
Copy link

codecov bot commented Mar 11, 2020

Codecov Report

Merging #482 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #482   +/-   ##
=======================================
  Coverage   98.42%   98.42%           
=======================================
  Files         104      104           
  Lines        3427     3427           
=======================================
  Hits         3373     3373           
  Misses         54       54

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 14b48cc...147b06c. Read the comment docs.

id: vars
run: |
make installdeps-test
export whitelist="pandas|numpy|scikit|xgboost|catboost|category-encoders|cloudpickle|dask|distributed|pyzmq|statsmodels"
Copy link
Contributor

@rwedge rwedge Mar 11, 2020

Choose a reason for hiding this comment

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

Some of this is repeated in the circleci workflow, yes? Maybe there should be a make action that generates the dependency text file that both the action and the workflow use.

Copy link
Contributor Author

@jeremyliweishih jeremyliweishih Mar 11, 2020

Choose a reason for hiding this comment

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

Yes the circleci workflow uses the same file to check for diffs whereas this github action generates the file! Does that make sense?

Copy link
Contributor

@rwedge rwedge Mar 11, 2020

Choose a reason for hiding this comment

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

I think in both cases a file is generated (albeit in different locations). We should avoid duplicating that code so it is easier to update the whitelist

Copy link
Contributor Author

@jeremyliweishih jeremyliweishih Mar 11, 2020

Choose a reason for hiding this comment

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

Got it! I'll add it in.

@angela97lin
Copy link
Contributor

angela97lin commented Mar 11, 2020

Pretty cool! Maybe related to @rwedge's comment but if we have this action, do we still need this in our circleCI workflow? :o That is, do we still need the CircleCI "check_dependencies_updated_linux" test?

@jeremyliweishih
Copy link
Contributor Author

jeremyliweishih commented Mar 11, 2020

Pretty cool! Maybe related to Roy's comment but if we have this action, do we still need this in our circleCI workflow? :o That is, do we still need the CircleCI "check_dependencies_updated_linux" test?

@angela97lin If the purpose of the "check_dependencies_updated_linux" test is to inform us that an error is is potentially caused by a dependency the "check_dependencies_updated_linux" test is still useful IMO.

@rwedge
Copy link
Contributor

rwedge commented Mar 11, 2020

@angela97lin also since this check would only run once a day it could miss dependencies updated since the last time the check ran

@angela97lin
Copy link
Contributor

angela97lin commented Mar 11, 2020

@jeremyliweishih @rwedge Thanks!

So the update to our workflow: We see that the check check_dependencies_updated_linux fails, we could wait until the next day where a PR is automatically generated or manually do it ourselves that day? If that's so, is it true to say that this primarily to cover us for updates that happen at night / while we're away?

- name: Create Pull Request
uses: Featurelabs/create-pull-request@v2
with:
token: ${{ secrets.GITHUB_TOKEN }}
commit-message: Update latest dependencies
title: Automated Latest Dependency Updates
body: This is an auto-generated PR with dependency updates.
branch: dep-update
base: master
reviewers: jeremyliweishih, angela97lin, dsherry
Copy link
Contributor

@jeff-hernandez jeff-hernandez Mar 11, 2020

Choose a reason for hiding this comment

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

Does this step need to be conditional? What happens if there are no dependency updates?

Copy link
Contributor Author

@jeremyliweishih jeremyliweishih Mar 11, 2020

Choose a reason for hiding this comment

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

create-pull-request handles that case by not doing anything.

@jeremyliweishih
Copy link
Contributor Author

jeremyliweishih commented Mar 11, 2020

@angela97lin if only the check_dependencies_updated_linux check fails we could do either but it shouldn't stop develop as its only showing that dependencies updated (but the updated dependencies didn't cause any problems). We can just move check_dependencies_updated_linux into a non-required check. This is mainly just to save the hassle of manually putting up a PR.

@rwedge
Copy link
Contributor

rwedge commented Mar 11, 2020

It would be interesting if a PR could be automatically created when the check_dependencies_updated_linux test fails as well, but there would need to be a strategy to avoid multiple automated PRs proposing the same changes

@angela97lin
Copy link
Contributor

angela97lin commented Mar 11, 2020

Gotcha! Hmm, is there a way to just run this action when a new PR is put up instead? Since that's usually when we'd notice this check fail and have to manually put up a PR 🤔

@jeremyliweishih
Copy link
Contributor Author

jeremyliweishih commented Mar 11, 2020

@angela97lin Theres no real support for manually running actions but I know a hacky way to do it (you set the trigger to watch and click the star when you need it but this won't be a good idea once we open source). I can add that in after this PR. I can also just make it run every couple hours (or as much as we want really).

@jeremyliweishih jeremyliweishih requested a review from rwedge Mar 11, 2020
name: Update Dependencies
on:
schedule:
- cron: '30 8 * * *'
Copy link
Contributor

@rwedge rwedge Mar 11, 2020

Choose a reason for hiding this comment

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

Do you want this to be 8:30 am eastern?

Copy link
Contributor Author

@jeremyliweishih jeremyliweishih Mar 12, 2020

Choose a reason for hiding this comment

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

yes ideally! Let me check how timezone works 😄

Copy link
Contributor Author

@jeremyliweishih jeremyliweishih Mar 12, 2020

Choose a reason for hiding this comment

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

good catch its on UTC @rwedge

rwedge
rwedge approved these changes Mar 12, 2020
Copy link
Contributor

@rwedge rwedge left a comment

I think the code looks good


.PHONY: dependenciesfile
dependenciesfile:
$(eval whitelist='pandas|numpy|scikit|xgboost|catboost|category-encoders|cloudpickle|dask|distributed|pyzmq|statsmodels')
Copy link
Collaborator

@dsherry dsherry Mar 13, 2020

Choose a reason for hiding this comment

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

Why use eval? Hmm was my use of export inappropriate?

Copy link
Collaborator

@dsherry dsherry Mar 13, 2020

Choose a reason for hiding this comment

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

I always forget how to set local variables in bash 😂

Copy link
Collaborator

@dsherry dsherry Mar 13, 2020

Choose a reason for hiding this comment

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

If this is in fact bash... hmm, how do our build scripts handle cross-platform support?

Copy link
Contributor Author

@jeremyliweishih jeremyliweishih Mar 13, 2020

Choose a reason for hiding this comment

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

It makes perfect sense for the .config file but there is some make vs. bash syntax (and how they interact) that I don't fully understand yet that prompted the use of this weird syntax.

body: This is an auto-generated PR with dependency updates.
branch: dep-update
base: master
reviewers: jeremyliweishih, angela97lin, dsherry
Copy link
Collaborator

@dsherry dsherry Mar 13, 2020

Choose a reason for hiding this comment

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

This is great. I wonder if there's a way to just say evalml-team or something. But no need to solve that now

Copy link
Contributor Author

@jeremyliweishih jeremyliweishih Mar 13, 2020

Choose a reason for hiding this comment

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

Yes that would work! I need to make a new team though - currently we have Feature Labs and Team Jeff 😄

Copy link
Collaborator

@dsherry dsherry Mar 13, 2020

Choose a reason for hiding this comment

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

Ooh then we should do that! How do we do that?

Copy link
Collaborator

@dsherry dsherry Mar 13, 2020

Choose a reason for hiding this comment

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

Doesn't need to happen in this PR if you don't wanna wait

Copy link
Collaborator

@dsherry dsherry left a comment

Great stuff! So cool to have this :)

name: Update Dependencies
on:
schedule:
- cron: '30 12 * * *'
Copy link
Collaborator

@dsherry dsherry Mar 13, 2020

Choose a reason for hiding this comment

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

Cool, I see this resolves to 12:30pm UTC, i.e. 8:30am EST. Just a note that this will shift an hour earlier from Nov-March with DST, lol!

@jeremyliweishih jeremyliweishih merged commit 91c2b3a into master Mar 13, 2020
2 checks passed
@dsherry dsherry deleted the 467_dependency_action branch Oct 29, 2020
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.

Dependency update checkin test: automate putting up PR on failure
5 participants