Skip to content

CI: split benchmarks from other checks #39392

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

Closed
wants to merge 1 commit into from

Conversation

afeld
Copy link
Member

@afeld afeld commented Jan 25, 2021

That job was the longest-running, so this should parallelize the various code checks with the benchmarks to get done in less wall clock time.

This pull request builds on #39745.

cc @fangchenli

  • closes #xxxx
  • tests added / passed
  • Ensure all linting tests pass, see here for how to run them
  • whatsnew entry

@afeld afeld added the CI Continuous Integration label Jan 25, 2021
@afeld afeld force-pushed the ci-cleanup branch 2 times, most recently from 943ba2c to 0423fa0 Compare January 25, 2021 08:24
Copy link
Contributor

@jreback jreback left a comment

Choose a reason for hiding this comment

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

not sure this is actually running we had 19 checks before, should show up in the list.

@@ -0,0 +1,13 @@
name: Set up pandas
Copy link
Contributor

Choose a reason for hiding this comment

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

can this be in more of the builds? (you change a few i c); normally do this as a separate PR (e.g. when you refactor want to do one thing), but ok to leave.

Copy link
Member Author

@afeld afeld Jan 25, 2021

Choose a reason for hiding this comment

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

can this be in more of the builds?

Maybe! The environment gets set up various ways for various builds. I wasn't sure why, so just changed the ones that looked the same.

normally do this as a separate PR

The refactoring was in service of splitting the two jobs, but happy to take that to a separate pull request if the splitting of the job doesn't work.

@jreback jreback added the Benchmark Performance (ASV) benchmarks label Jan 25, 2021
@afeld afeld force-pushed the ci-cleanup branch 4 times, most recently from 2573f02 to 93a2de2 Compare January 25, 2021 14:56
@afeld
Copy link
Member Author

afeld commented Jan 25, 2021

not sure this is actually running we had 19 checks before

Ah, whoops, I thought that was a GitHub Actions glitch, but as usual, Problem Exists Between Chair and Keyboard 😛 Hadn't realized that when using an action in the same repository:

  • The path needs to start with ./
  • The job needs to run actions/checkout before that action can be used (which makes sense, now that I think about it)
  • Need to specify the shell for every step (defaults aren't inherited)

20 checks running now, as intended!

@jreback
Copy link
Contributor

jreback commented Feb 11, 2021

@afeld can you merge master

@afeld afeld force-pushed the ci-cleanup branch 2 times, most recently from 8ff53e6 to d3230d0 Compare February 11, 2021 03:52
@afeld afeld mentioned this pull request Feb 11, 2021
4 tasks
@afeld afeld force-pushed the ci-cleanup branch 3 times, most recently from e864731 to 259a359 Compare February 11, 2021 08:32
@afeld afeld requested a review from jreback February 11, 2021 08:34
@jreback jreback added this to the 1.3 milestone Feb 11, 2021
@jreback
Copy link
Contributor

jreback commented Feb 11, 2021

can you merge master

@jreback
Copy link
Contributor

jreback commented Feb 11, 2021

i merged this one: #39745

@afeld
Copy link
Member Author

afeld commented Feb 12, 2021

Hmm, went from the longest run being CI / Checks at 48 minutes to CI / Benchmarks taking 46. Better, but not a lot better 😕

Flaky tests failing, but CI ones are 🟢.

@github-actions
Copy link
Contributor

This pull request is stale because it has been open for thirty days with no activity. Please update or respond to this comment if you're still interested in working on this.

@github-actions github-actions bot added the Stale label Mar 15, 2021
That job was the longest-running, so this should parallelize them.
@afeld afeld added the Refactor Internal refactoring of code label Mar 19, 2021
@jbrockmendel
Copy link
Member

@afeld is this ready?

@datapythonista
Copy link
Member

Not sure if this is worth. We've got an overheat of 15 minutes, and we are saving 5 from 50 for an extra build. No strong opinion, but I think our CI should be faster without this with the amount of builds we usually have running (unless we've got many more than the 20 GH actions jobs, that I don't think we did).

@simonjayhawkins simonjayhawkins removed this from the 1.3 milestone May 24, 2021
@datapythonista
Copy link
Member

Not sure if this is worth. We've got an overheat of 15 minutes, and we are saving 5 from 50 for an extra build. No strong opinion, but I think our CI should be faster without this with the amount of builds we usually have running (unless we've got many more than the 20 GH actions jobs, that I don't think we did).

@afeld thoughts on this?

@simonjayhawkins
Copy link
Member

@afeld closing as stale. feel free to reopen if you want to continue with this. (@datapythonista has a question)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Benchmark Performance (ASV) benchmarks CI Continuous Integration Refactor Internal refactoring of code Stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants