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

Add code quality checks to GitHub Actions #1453

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

Cyber-Var
Copy link

Addresses: Good First Issue #663

Changes made:

  • Removed trailing whitespaces where necessary (excluding .md and hidden files).
  • And removed unnecessary non-ASCII symbols (also excluding .md and hidden files).
  • Added a new job (code-quality-checks) within the GitHub Actions workflow that runs 3 checks automatically (it checks for trailing whitespaces and non-ASCII symbols in files and filenames excluding those ones where it's expected.

@github-actions github-actions bot added category: llm_bench Label for tool/llm_bench folder category: text to image Text 2 image pipeline category: visual language Visual language pipeline category: continuous batching Continuous batching category: LLM LLM pipeline (stateful, static) category: whisper Whisper pipeline category: sampling Sampling / Decoding algorithms category: speculative decoding Speculative decoding category: LoRA Low rank adapters category: GHA CI based on Github actions category: cmake / build Cmake scripts category: tokenizers Tokenizer class or submodule update category: Python API Python API for GenAI category: samples GenAI samples category: GenAI C++ API Changes in GenAI C++ public headers no-match-files category: prompt lookup labels Dec 30, 2024
- name: Prohibit non-ASCII characters in file names
run: test $(git diff --name-only --diff-filter=A -z $(git hash-object -t tree /dev/null) | LC_ALL=C tr -d '[ -~]\0' | wc -c) == 0
- name: Check for non-ASCII characters in files (excluding Markdown and hidden files), with characters ± and � allowed
run: ! git grep -n '[^ -~±�]' -- $(git ls-files | grep -vE '^\.' | grep -vE '\.md$' | grep -vE '^(tests/python_tests|tools/who_what_benchmark/(tests|whowhatbench))' | grep -v 'tools/llm_bench/llm_bench_utils/ov_model_classes.py')
Copy link
Contributor

Choose a reason for hiding this comment

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

is it possible to use some existing tools instead of manual commands?

Copy link
Author

Choose a reason for hiding this comment

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

I will look for some

Copy link
Author

Choose a reason for hiding this comment

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

@ilya-lavrenov , I've updated the pull request.

The causal_lm_cpp.yml workflow now uses .pre-commit-config.yaml for running pre-commit hooks instead of manual commands. They:
• Remove trailing whitespaces
• Ensure files end with a newline

Additionally, I've added new pre-commit hooks to:
• Validate JSON files for syntax correctness
• Detect merge conflict markers

For non-ASCII character checks, I've introduced custom scripts located in the pre_commit_scripts folder, as I couldn't find suitable existing tools. These scripts are integrated and managed through the same .pre-commit-config.yaml.

Please review the changes and let me know if further adjustments are needed.

Copy link
Collaborator

@Wovchena Wovchena left a comment

Choose a reason for hiding this comment

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

I'm OK with pre-commit usage

.pre-commit-config.yaml Outdated Show resolved Hide resolved
.pre-commit-config.yaml Outdated Show resolved Hide resolved
Copy link
Collaborator

@Wovchena Wovchena left a comment

Choose a reason for hiding this comment

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

Thank you! Let's wait for @ilya-lavrenov's review. You'll also need to resolve the conflicts.

@Wovchena Wovchena requested a review from ilya-lavrenov January 6, 2025 10:50
@Cyber-Var
Copy link
Author

@Wovchena, thank you for the approval! I'll address the conflicts.

…ead of manual commands. Configured pre-commit with automated code quality checks and custom scripts for non-ASCII checks.
…-of-file-fixer hook, and to only exclude git-related hidden files from all hooks.
@Cyber-Var Cyber-Var force-pushed the add-code-quality-checks branch from 2b33b01 to 5b000a8 Compare January 6, 2025 11:18
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is automatically generated file. It causes build error.

CMake Error at /home/runner/work/openvino.genai/openvino.genai/src/python/compare_pyi.cmake:25 (message):
File
/home/runner/work/openvino.genai/openvino.genai/src/python/openvino_genai/py_openvino_genai.pyi
is outdated and need to be regenerated with pybind11-stubgen

When GenAI is compiled this file is regenerated. Try installing pybind11-stubgen==2.5.1 and compiling genai. That should modify src/python/openvino_genai/py_openvino_genai.pyi. Push that new file version

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category: cmake / build Cmake scripts category: continuous batching Continuous batching category: GenAI C++ API Changes in GenAI C++ public headers category: GHA CI based on Github actions category: llm_bench Label for tool/llm_bench folder category: LLM LLM pipeline (stateful, static) category: LoRA Low rank adapters category: prompt lookup category: Python API Python API for GenAI category: samples GenAI samples category: sampling Sampling / Decoding algorithms category: speculative decoding Speculative decoding category: text to image Text 2 image pipeline category: tokenizers Tokenizer class or submodule update category: visual language Visual language pipeline category: whisper Whisper pipeline no-match-files
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants