Skip to content

Conversation

wangxiyuan
Copy link
Collaborator

@wangxiyuan wangxiyuan commented Aug 29, 2025

What this PR does / why we need it?

Does this PR introduce any user-facing change?

How was this patch tested?

Signed-off-by: wangxiyuan <wangxiyuan1007@gmail.com>
Copy link
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 updates the base Docker image in Dockerfile.openEuler to use openEuler 24.03. However, this change introduces an inconsistency, as other Dockerfiles in the repository, such as Dockerfile.310p.openEuler and Dockerfile.a3.openEuler, have not been updated and still use the older openeuler22.03 base image. To ensure consistent build and runtime environments across all supported hardware targets, it is highly recommended to update all related Dockerfiles to the new base image version.

#

FROM quay.io/ascend/cann:8.2.rc1-910b-openeuler22.03-py3.11
FROM quay.io/ascend/cann:8.2.rc1-910b-openeuler24.03-py3.11
Copy link
Contributor

Choose a reason for hiding this comment

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

high

This update to openeuler24.03 is only applied to this Dockerfile. Other Dockerfiles in the project, such as Dockerfile.310p.openEuler and Dockerfile.a3.openEuler, still use openeuler22.03. This creates an inconsistent environment across different build targets, which can lead to subtle bugs and increased maintenance overhead. It is strongly recommended to update all related Dockerfiles to use the same base OS version for consistency.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@gemini-code-assist this change is just for test. I'll update all the related file once the test works

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for the clarification. It's understandable to make such changes for testing purposes. Please ensure that all relevant Dockerfiles are updated for consistency once your testing is complete and the change is ready for final integration. This will help maintain a stable and predictable environment across the project.

Copy link

👋 Hi! Thank you for contributing to the vLLM Ascend project. The following points will speed up your PR merge:‌‌

  • A PR should do only one thing, smaller PRs enable faster reviews.
  • Every PR should include unit tests and end-to-end tests ‌to ensure it works and is not broken by other future PRs.
  • Write the commit message by fulfilling the PR description to help reviewer and future developers understand.

If CI fails, you can run linting and testing checks locally according Contributing and Testing.

@wangxiyuan wangxiyuan closed this Aug 29, 2025
@wangxiyuan wangxiyuan deleted the upgrade_2403 branch September 4, 2025 06:52
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