ci: Remove mypy deps install step in python_cache action #3956
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
silvanocerza
requested review from
vblagoje and
bogdankostic
and removed request for
a team
|
Somehow related to #3930. |
masci
reviewed
Jan 26, 2023
| echo "changed_files=$changed_files" >> $GITHUB_OUTPUT | ||
| with: | ||
| files: | | ||
| *.py |
There was a problem hiding this comment.
I think we can ignore files under test/ to make the job even faster, see https://github.com/deepset-ai/haystack/pull/3930/files#diff-1db27d93186e46d3b441ece35801b244db8ee144ff1405ca27a163bfe878957f
There was a problem hiding this comment.
Right, I forgot. Adding that.
masci
reviewed
Jan 26, 2023
| @@ -109,7 +103,7 @@ jobs: | |||
| if: steps.python-files.outputs.changed_files | |||
| run: | | |||
| mkdir .mypy_cache/ | |||
| mypy ${{ steps.python-files.outputs.changed_files }} | |||
| mypy --install-types --non-interactive ${{ steps.python-files.outputs.changed_files }} --exclude=rest_api/build/ --exclude=rest_api/test/ | |||
There was a problem hiding this comment.
nit: you don't need to exclude anything at this point (provided you filter out changed files)?
There was a problem hiding this comment.
True that, but might as well leave it I guess? Better safe than sorry. 🤷
6 tasks
6 tasks
ZanSara
pushed a commit
that referenced
this pull request
Jan 27, 2023
* Remove mypy deps install step in python_cache action * Remove step caching mypy dependencies * Add ignore files in changed files retrieval step
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Related Issues
https://github.com/deepset-ai/haystack/actions/runs/4014088908/jobs/6894191362
Proposed Changes:
Remove the step that install
mypydeps in thepython_cacheaction.mypydeps installation is now done directly when invoked on modified*.pyfiles, this should also make it faster since it only installs depedencies necessary to test those files.This PR also removes the
mypycaching step inpython_cacheaction as there would be nothing to cache.How did you test it?
I didn't, it's CI stuff. I just tested that the commands are correct by running them locally first.
Notes for the reviewer
The step that install
mypydeps inpython_cachewas called with both--install-typesand--non-interactiveflags, as documented here those two flags used in combination forces type checking the code.For more info about the individual flags see
--install-typesand--non-interactivedocumentation.Checklist
I have updated the related issue with new insights and changesI added tests that demonstrate the correct behavior of the changefix:,feat:,build:,chore:,ci:,docs:,style:,refactor:,perf:,test:.I documented my code