Combine file checks with build checks back into a single workflow file#1058
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request replaces a YAML lint problem matcher with a new configuration for yapf diffs. A review comment correctly identified that the regular expression for parsing diff hunk headers was too restrictive, as it failed to account for cases where line counts are omitted in single-line changes; a suggestion was provided to make the line range optional.
The script `ci_validate_tutorials.sh` installs it, and besides, the version installed by this `ci.yaml` didn't match the script's version.
The last step was meant to call `scripts/format_ipynb.sh`, but that script doesn't check the format; it reformats the files. So, let's skip that last step.
There's not enough cause in the way this will be used.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request replaces the yamllint problem matcher with a new configuration for yapf-diff. The new yapf.json file contains invalid JSON due to unescaped backslashes in regular expressions and lacks necessary properties like severity and loop, which are required for the problem matcher to function correctly across multiple hunks.
It doesn't work to set it globally.
It doesn't have any effect.
On re-think, decided they are not worth it for this project.
1 participant
Previously, I tried to make ci-file-checks.yaml be efficient and skip running some checks when they were not relevant to the files changed in a PR. However, the maintenance has been higher than hoped, and other team members didn't like the arrangement. Moreover, the overall CI workflow execution time for TFQ is bounded by the build checks, so skipping some file checks did not reduce overall CI time. While it is obviously best to save CPU cycles (= electricity = energy) as much as possible by not running unnecessary programs, the tradeoffs still argue against keeping the current ci-file-checks.yaml approach.
So, this PR:
ci.yaml