Skip to content

[build] declare kvcm_ops wheel runtime dependency - #304

Open
wangxiyu191 wants to merge 1 commit into
mainfrom
harness/5174c093
Open

[build] declare kvcm_ops wheel runtime dependency#304
wangxiyu191 wants to merge 1 commit into
mainfrom
harness/5174c093

Conversation

@wangxiyu191

Copy link
Copy Markdown
Collaborator

Summary

  • Declare requests==2.32.5 as a runtime dependency of the kvcm_ops wheel.
  • Reuse a shared requirements fragment for Bazel dependency locking and wheel metadata.
  • Add wheel metadata and clean-venv installation regression coverage to the normal CI suite.

Tests

  • bazelisk test //package/kvcm_ops/... --config=ci_fast --cache_test_results=no --test_output=errors
  • bazelisk test //open_source/deps:requirements_cpu_test --cache_test_results=no --test_output=errors

@qoderai qoderai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Overall this looks like a clean, focused fix: it centralizes the requests requirement, declares it as a runtime dependency of the kvcm_ops wheel, and adds regression coverage for wheel metadata and installation.

Notes / questions:

  • Verified that the pinned rules_python commit (084b877c) already supports requires_file on py_wheel, so the new attribute is safe.
  • open_source/deps/BUILD correctly adds requirements_kvcm_ops.txt to exports_files and extra_data, which is needed now that requirements_base.txt includes it via -r.
  • The lock-file annotation changed to # via -r open_source/deps/././requirements_kvcm_ops.txt. This is only cosmetic, but make sure it was produced by the project's pinned pip-compile invocation and not hand-edited; otherwise regenerating the lock file may revert it.
  • wheel_install_test.py creates a clean venv and installs from PyPI. As noted inline, this introduces a network dependency and a requirement that the Bazel Python runtime has ensurepip. If CI runners are sometimes firewalled or use a stripped Python, this could become flaky.
  • Minor: the typo in the assertion string ("kvcm: list_intance.") matches the existing source, so the test is consistent; no change needed unless you want to fix the source separately.

🤖 Generated by Qoder

self.assertEqual(0, result.returncode, msg=result.stdout)
return result.stdout

def test_install_resolves_requests_and_starts_http_command(self):

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This is a useful regression test, but be aware it depends on the CI runner having outbound PyPI access and a base Python with ensurepip. If either is unavailable, the normal_test shard will fail. Consider documenting this requirement or adding tags = ["manual"] if network access is not guaranteed.


🤖 Generated by Qoder

@github-actions github-actions Bot added the ai reviewed AI has reviewed this PR label Aug 24, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ai reviewed AI has reviewed this PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant