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

Use CircleCI Matrix Jobs #1043

Merged
merged 16 commits into from Aug 12, 2020
Merged

Use CircleCI Matrix Jobs #1043

merged 16 commits into from Aug 12, 2020

Conversation

gsheni
Copy link
Member

@gsheni gsheni commented Aug 11, 2020

Use CircleCI Matrix Jobs

  • Use CircleCI matrix jobs in config to trigger multiple runs of same job with different parameters

@gsheni gsheni self-assigned this Aug 11, 2020
@gsheni gsheni changed the title Use CircleCI Matrix Jobs [WIP] Use CircleCI Matrix Jobs Aug 11, 2020
@gsheni gsheni changed the title [WIP] Use CircleCI Matrix Jobs Use CircleCI Matrix Jobs Aug 11, 2020
@gsheni gsheni requested a review from dsherry Aug 11, 2020
@gsheni
Copy link
Member Author

gsheni commented Aug 11, 2020

Verified that Proper CI tests are being run:

Screen Shot 2020-08-11 at 5 42 21 PM

Screen Shot 2020-08-11 at 5 42 25 PM

condition: << parameters.codecov >>
condition:
and:
- equal: [ "3.8", << parameters.python_version >> ]
Copy link
Member Author

@gsheni gsheni Aug 11, 2020

Choose a reason for hiding this comment

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

Only runs codecov when python == 3.8 and core_dependencies is true

Copy link
Collaborator

@dsherry dsherry Aug 12, 2020

Choose a reason for hiding this comment

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

Why is this change necessary?

I think its fine to keep the existing code here, and simply not configure a 3.6 or 3.7 job to run codecov in the jobs section

I would prefer the jobs section below spells out the behavior of each job, rather than having to read the code inside each job (i.e. this updated check) in order to understand the behavior.

Copy link
Collaborator

@dsherry dsherry Aug 12, 2020

Choose a reason for hiding this comment

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

@gsheni , on reflection, this change is incorrect and needs to be deleted.

We have unit tests which only run if core_dependencies==False, and other unit tests which only run if core_dependencies==True. So, we need to run codecov on python 3.8, both with and without core_dependencies set to True, because then when we upload both reports to codecov, they get merged. Without this, codecov will fail cryptically, as it has done on this PR.

Copy link
Member Author

@gsheni gsheni Aug 12, 2020

Choose a reason for hiding this comment

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

Changed the code to

            matrix:
              parameters:
                python_version: ["3.8"]
                core_dependencies: [false, true]
                codecov: [true]
  • So codecove runs with both core true, false on 3.8

@codecov
Copy link

codecov bot commented Aug 11, 2020

Codecov Report

Merging #1043 into main will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1043   +/-   ##
=======================================
  Coverage   99.91%   99.91%           
=======================================
  Files         183      183           
  Lines       10147    10147           
=======================================
  Hits        10138    10138           
  Misses          9        9           

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 2cbb312...c883e76. Read the comment docs.

Copy link
Collaborator

@dsherry dsherry left a comment

@gsheni thanks for contributing this. I let a couple comments, will approve once we resolve those.

parameters:
python_version: ["3.8"]
core_dependencies: [false, true]
name: "linux python << matrix.python_version >> unit tests, core dependencies << matrix.core_dependencies >>"
Copy link
Collaborator

@dsherry dsherry Aug 12, 2020

Choose a reason for hiding this comment

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

@gsheni can we reorganize this so that we run 3.6/3.7/3.8 with core_dependencies=false, then run 3.8 with core_dependencies=True as a normal, non-matrix job?

Copy link
Member Author

@gsheni gsheni Aug 12, 2020

Choose a reason for hiding this comment

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

Fixed

docs/source/release_notes.rst Outdated Show resolved Hide resolved
condition: << parameters.codecov >>
condition:
and:
- equal: [ "3.8", << parameters.python_version >> ]
Copy link
Collaborator

@dsherry dsherry Aug 12, 2020

Choose a reason for hiding this comment

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

Why is this change necessary?

I think its fine to keep the existing code here, and simply not configure a 3.6 or 3.7 job to run codecov in the jobs section

I would prefer the jobs section below spells out the behavior of each job, rather than having to read the code inside each job (i.e. this updated check) in order to understand the behavior.

@@ -53,7 +52,6 @@ jobs:
parameters:
python_version:
type: string
default: "3.8"
Copy link
Collaborator

@dsherry dsherry Aug 12, 2020

Choose a reason for hiding this comment

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

Just to confirm: deleting these doesn't change the current behavior at all, does it?

Copy link
Member Author

@gsheni gsheni Aug 12, 2020

Choose a reason for hiding this comment

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

No, this doesn't. It just makes it so you have to specific the python version when running this job. If you don't specific it, the job will error, and CI will fail with missing parameter error

default: "3.8"
codecov:
type: boolean
default: false
Copy link
Collaborator

@dsherry dsherry Aug 12, 2020

Choose a reason for hiding this comment

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

Why delete the default for codecov?

Copy link
Member Author

@gsheni gsheni Aug 12, 2020

Choose a reason for hiding this comment

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

Put this back in.

@dsherry
Copy link
Collaborator

dsherry commented Aug 12, 2020

I like the way the parameters always appear in the names of the test jobs now, that's helpful

Screen Shot 2020-08-12 at 10 27 28 AM

@gsheni
Copy link
Member Author

gsheni commented Aug 12, 2020

@dsherry I put back the default arguments for the jobs. It seems that CircleCI matrix is fine with them having them in. In addition, I made it so codecov runs with core True and False on python 3.8
Screen Shot 2020-08-12 at 11 40 44 AM

python_version: ["3.8"]
core_dependencies: [false, true]
codecov: [true]
name: "linux python << matrix.python_version >> unit tests, core dependencies << matrix.core_dependencies >>, codecov << matrix.codecov >>"
Copy link
Collaborator

@dsherry dsherry Aug 12, 2020

Choose a reason for hiding this comment

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

Ohhhh, @gsheni , seeing this now, I realize I gave you a bad recommendation in my change request. Sorry about that.

This code currently runs the following jobs:

  • python 3.6 lint test - Success
  • python 3.7 lint test - Success
  • python 3.8 lint test - Success
  • linux python 3.6 unit tests, core dependencies false, codecov false - Success
  • linux python 3.7 unit tests, core dependencies false, codecov false - Success
  • linux python 3.8 unit tests, core dependencies false, codecov false - Success
  • linux python 3.8 unit tests, core dependencies false, codecov true - Success
  • linux python 3.8 unit tests, core dependencies true, codecov true - Success

Note the last 3 jobs all run python 3.8. We should not run this one, because it's unnecessary:

  • linux python 3.8 unit tests, core dependencies false, codecov false - Success

So what I think we should do here

        - unit_tests:
            matrix:
              parameters:
                python_version: ["3.6", "3.7"]
                core_dependencies: [false]
            name: "linux python << matrix.python_version >> unit tests, core dependencies << matrix.core_dependencies >>, codecov false"
        - unit_tests:
            matrix:
              parameters:
                python_version: ["3.8"]
                core_dependencies: [false, true]
                codecov: [true]
            name: "linux python << matrix.python_version >> unit tests, core dependencies << matrix.core_dependencies >>, codecov << matrix.codecov >>"

Copy link
Member Author

@gsheni gsheni Aug 12, 2020

Choose a reason for hiding this comment

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

Fixed.

Copy link
Collaborator

@dsherry dsherry left a comment

Approved, once we resolve one more comment which I left

core_dependencies: [false]
name: "linux python << matrix.python_version >> unit tests, core dependencies << matrix.core_dependencies >>, codecov false"
codecov: [false]
Copy link
Member Author

@gsheni gsheni Aug 12, 2020

Choose a reason for hiding this comment

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

This way someone in the future can explicitly see the argument being passed

Copy link
Collaborator

@dsherry dsherry left a comment

Thanks!

@gsheni gsheni merged commit b27b18b into main Aug 12, 2020
@gsheni gsheni deleted the matrix_circleci branch Aug 12, 2020
@dsherry dsherry mentioned this pull request Aug 25, 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.

None yet

2 participants