Skip to content

ci: add pip/pre-commit cache and fix mirror override in NPU CI#9320

Open
JavaPythonAIForBAT wants to merge 1 commit into
modelscope:mainfrom
JavaPythonAIForBAT:ci/add-cache-optimization
Open

ci: add pip/pre-commit cache and fix mirror override in NPU CI#9320
JavaPythonAIForBAT wants to merge 1 commit into
modelscope:mainfrom
JavaPythonAIForBAT:ci/add-cache-optimization

Conversation

@JavaPythonAIForBAT
Copy link
Copy Markdown

Summary

  • lint.yaml: upgrade actions/setup-python to v5 with built-in cache: pip enabled; add actions/cache@v4 for pre-commit environments keyed on .pre-commit-config.yaml hash
  • citest_npu.yaml: add actions/cache@v4 for ~/.cache/pip keyed on requirements/framework.txt and requirements/tests.txt hashes, reducing redundant pip downloads across runs
  • ci_container_test.sh: remove explicit -i https://mirrors.aliyun.com/pypi/simple/ flags from all pip install commands so they use the internal cache server already configured in the Config mirrors step, rather than overriding it

Motivation

The Config mirrors step in citest_npu.yaml correctly sets the internal K8s pip cache service via pip config set global.index-url, but ci_container_test.sh was overriding this with explicit -i flags pointing to the public Aliyun mirror. This PR fixes the inconsistency and adds standard pip/pre-commit caching to speed up CI.

Test plan

  • Lint workflow: verify pre-commit cache is restored on subsequent runs
  • NPU CI: verify pip cache is restored and ci_container_test.sh uses the configured index URL

- lint.yaml: upgrade setup-python to v5 with pip cache enabled; add
  pre-commit environment cache keyed on .pre-commit-config.yaml
- citest_npu.yaml: add actions/cache@v4 for ~/.cache/pip keyed on
  requirements/framework.txt and requirements/tests.txt hashes
- ci_container_test.sh: remove explicit -i mirror flags so all pip
  installs use the internal cache server configured in Config mirrors

Co-Authored-By: wuhejun <wuhejun@h-partners.com>
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request removes the Aliyun PyPI mirror from the pip install commands within the CI container test script. The reviewer suggests enhancing the script's reliability by adding error handling to the installation steps to prevent the CI from proceeding if dependencies fail to install. Furthermore, combining multiple pip install calls into a single command was recommended for better efficiency and dependency resolution.

if [ "$MODELSCOPE_SDK_DEBUG" == "True" ]; then
# pip config set global.index-url https://pypi.tuna.tsinghua.edu.cn/simple
pip install -r requirements/tests.txt -i https://mirrors.aliyun.com/pypi/simple/
pip install -r requirements/tests.txt
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

It is recommended to add error handling to the pip install command. Without it, if the installation of test dependencies fails, the script will continue to subsequent steps (like pre-commit), which may lead to misleading failures or unreliable test results in CI.

Suggested change
pip install -r requirements/tests.txt
pip install -r requirements/tests.txt || exit 1

Comment on lines +23 to +24
pip install -r requirements/framework.txt -U
pip install decord einops -U
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

These two pip install commands can be combined into a single invocation. This is more efficient as it reduces the overhead of starting the pip process multiple times and allows the dependency resolver to handle all packages together, ensuring consistency. Additionally, adding || exit 1 ensures the script terminates immediately if the installation fails.

Suggested change
pip install -r requirements/framework.txt -U
pip install decord einops -U
pip install -r requirements/framework.txt decord einops -U || exit 1

# test with install
pip install .
pip install auto_gptq bitsandbytes deepspeed -U -i https://mirrors.aliyun.com/pypi/simple/
pip install auto_gptq bitsandbytes deepspeed -U
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

Adding error handling here ensures that the script does not proceed to run the tests if these dependencies fail to install, which helps in identifying environment setup issues early and prevents running tests in an incomplete environment.

Suggested change
pip install auto_gptq bitsandbytes deepspeed -U
pip install auto_gptq bitsandbytes deepspeed -U || exit 1

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.

1 participant