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

Adjust CI scripts for package specific tasks #139

Merged
merged 18 commits into from
Nov 1, 2023
Merged

Conversation

maxschulz-COL
Copy link
Contributor

@maxschulz-COL maxschulz-COL commented Oct 31, 2023

Description

This PR adjusts the CI such that:

  • linting is always done for all PRs
  • checks for schema and changelog fragments are only applied if there are changes in the relevant folder of the package
  • unit and integration tests are only applied if there are changes in the src folder of the relevant package (EDIT: this is reverted and not relevant anymore)

Screenshot

Checklist

  • I have not referenced individuals, products or companies in any commits, directly or indirectly
  • I have not added data or restricted code in any commits, directly or indirectly
  • I have updated the docstring of any public function/class/model changed
  • I have added tests to cover my changes (if applicable)

Types of changes

  • Docs/refactoring (non-breaking change which improves codebase)
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Notice

  • I acknowledge and agree that, by checking this box and clicking "Submit Pull Request":

    • I submit this contribution under the Apache 2.0 license and represent that I am entitled to do so on behalf of myself, my employer, or relevant third parties, as applicable.
    • I certify that (a) this contribution is my original creation and / or (b) to the extent it is not my original creation, I am authorized to submit this contribution on behalf of the original creator(s) or their licensees.
    • I certify that the use of this contribution as authorized by the Apache 2.0 license does not violate the intellectual property rights of anyone else.

@maxschulz-COL maxschulz-COL changed the title Test PR - Do not merge Adjust CI scripts for package specific tasks Nov 1, 2023
Copy link
Contributor

@huong-li-nguyen huong-li-nguyen left a comment

Choose a reason for hiding this comment

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

The checks-vizro-ai.yml needs to be updated for the changelog command - we changed it in checks-vizro-core.yml a few days ago

.github/workflows/checks-vizro-ai.yml Show resolved Hide resolved
@antonymilne
Copy link
Contributor

  • linting is always done for all PRs

Why do we want this? Isn't it better to run linting just on the relevant vizro-core vs. vizro-ai directory?

@maxschulz-COL
Copy link
Contributor Author

  • linting is always done for all PRs

Why do we want this? Isn't it better to run linting just on the relevant vizro-core vs. vizro-ai directory?

In principle yes, but there are a few complications:

  • it is actually fairly annoying to code up (but manageable)
  • what about changes outside either of those folders

Given that linting comes cheaply, and I have seen it cause bugs in mono-repos before (when only certain things are considered), I would prefer just to keep this general

@antonymilne
Copy link
Contributor

antonymilne commented Nov 1, 2023

In principle yes, but there are a few complications:

  • it is actually fairly annoying to code up (but manageable)
  • what about changes outside either of those folders

Given that linting comes cheaply, and I have seen it cause bugs in mono-repos before (when only certain things are considered), I would prefer just to keep this general

OK, fair enough. The situations I want to avoid are:

  • my PR in vizro-core fails checks due to some random linting error in vizro-ai which I didn't even touch
  • my PR in vizro-core takes significantly longer to be checked due to linting running on vizro-ai

(plus the equivalent situations with vizro-core and vizro-ai swapped round, obviously)

But so long as our linting is fast and we keep the pre-commit hook versions stable (so that random errors don't start popping up due to linter version changes) then the above situations shouldn't arise, so fine how you have it.

Comment on lines +42 to +47

- name: List dependencies
run: hatch run all.py${{ matrix.python-version }}:pip freeze

- name: Lint
run: hatch run all.py${{ matrix.python-version }}:lint
Copy link
Contributor

Choose a reason for hiding this comment

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

Since linting just requires pre-commit actually we should probably make a new deatched=True Hatch environment lint for it that doesn't include all of the dev dependencies. This way we could just do hatch run lint:lint on CI and it would be significantly faster. At the moment we install all the dependencies for all.py3.8 and then run pre-commit that uses only one of those dependencies which is a bit silly.

Not a big deal though, so feel free to do in a separate PR another time if you prefer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm good idea, let me see if I can incorporate that quickly still

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On the other hand, hatch run lint is really used a lot, so would prefer not to have hatch run lint:lint. Or do you mean to keep pre-commit in the default environment as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's revisit in another PR if we still want to do changes

Copy link
Contributor

Choose a reason for hiding this comment

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

@maxschulz-COL yes, you're right - I originally put lint in the default environment precisely so that we don't have to write hatch run lint:lint. But probably the right way to do it is to make the hatch run lint:lint the "correct" command with a detached lint environment and then put a script "lint" = ["hatch run lint:lint"] in the default environment to just act as a shortcut so you can still do hatch run lint.

@maxschulz-COL
Copy link
Contributor Author

In principle yes, but there are a few complications:

  • it is actually fairly annoying to code up (but manageable)
  • what about changes outside either of those folders

Given that linting comes cheaply, and I have seen it cause bugs in mono-repos before (when only certain things are considered), I would prefer just to keep this general

OK, fair enough. The situations I want to avoid are:

  • my PR in vizro-core fails checks due to some random linting error in vizro-ai which I didn't even touch
  • my PR in vizro-core takes significantly longer to be checked due to linting running on vizro-ai

(plus the equivalent situations with vizro-core and vizro-ai swapped round, obviously)

But so long as our linting is fast and we keep the pre-commit hook versions stable (so that random errors don't start popping up due to linter version changes) then the above situations shouldn't arise, so fine how you have it.

Agreed, but since we most importantly do share the same pre-commit config (and hence linter version) the first should not arise. And the second should not really be a problem, we're still talking order of couple of seconds

@maxschulz-COL maxschulz-COL merged commit 598823e into main Nov 1, 2023
16 checks passed
@maxschulz-COL maxschulz-COL deleted the test/check_CI branch November 1, 2023 14:12
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.

3 participants