Skip to content

fix(clp-package): Remove duplicate PYTHONPATH from the container start command (fixes #1513).#1545

Merged
junhaoliao merged 1 commit into
y-scope:mainfrom
junhaoliao:remove-dup-python-path
Nov 4, 2025
Merged

fix(clp-package): Remove duplicate PYTHONPATH from the container start command (fixes #1513).#1545
junhaoliao merged 1 commit into
y-scope:mainfrom
junhaoliao:remove-dup-python-path

Conversation

@junhaoliao

@junhaoliao junhaoliao commented Nov 4, 2025 •
edited by coderabbitai Bot
Loading

Copy link
Copy Markdown
Member

Description

As the title says.

Checklist

  • The PR satisfies the contribution guidelines.
  • This is a breaking change and that has been indicated in the PR title, OR this isn't a
    breaking change.
  • Necessary docs have been updated, OR no docs need to be updated.

Validation performed

task

cd build/clp-package
./sbin/start-clp.sh
./sbin/compress.sh ~/samples/hive-24hr
# observed the compression was successful

./sbin/search.sh 123456
# observed results were returned successfully

Summary by CodeRabbit

  • Refactor
    • Optimized Docker command generation by consolidating environment variable configuration. The PYTHONPATH setting is maintained through existing mechanisms, eliminating redundant specification while preserving container behaviour and final state.

@junhaoliao junhaoliao requested a review from a team as a code owner November 4, 2025 01:40
@coderabbitai

coderabbitai Bot commented Nov 4, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

Walkthrough

The change removes a duplicate explicit PYTHONPATH environment variable specification from the docker run command construction in generate_container_start_cmd. The PYTHONPATH remains set through the environment variables dictionary appended to the command, resulting in no observable change to container environment or behaviour.

Changes

Cohort / File(s) Summary
Docker command cleanup
components/clp-package-utils/clp_package_utils/general.py		 Removed duplicate -e "PYTHONPATH=..." argument from docker run command; PYTHONPATH now only specified via environment variables dictionary

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

  • Straightforward removal of duplicate environment variable specification with no functional impact

Possibly related issues

Pre-merge checks and finishing touches

✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change: removing a duplicate PYTHONPATH from the container start command, which directly matches the changeset modifications.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

junhaoliao
junhaoliao commented Nov 4, 2025
View reviewed changes
Comment thread components/clp-package-utils/clp_package_utils/general.py
"--rm",
"--network", "host",
"-w", str(CONTAINER_CLP_HOME),
"-e", f"PYTHONPATH={clp_site_packages_dir}",

@junhaoliao junhaoliao Nov 4, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

this duplicates the definition at L405 and therefore should be removed

@LinZhihao-723 LinZhihao-723 changed the title fix(clp-package): Remove duplicate PYTHONPATH from the container start command (fixes #1513). fix(clp-package): Remove duplicate PYTHONPATH from the container start command (fixes #1513). Nov 4, 2025
LinZhihao-723
LinZhihao-723 approved these changes Nov 4, 2025
View reviewed changes

@LinZhihao-723 LinZhihao-723 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Directly modified the PR title.

@junhaoliao junhaoliao merged commit ad7cffe into y-scope:main Nov 4, 2025
22 checks passed
@junhaoliao junhaoliao deleted the remove-dup-python-path branch May 7, 2026 19:46
junhaoliao added a commit to junhaoliao/clp that referenced this pull request May 17, 2026
@junhaoliao
fix(clp-package): Remove duplicate PYTHONPATH from the container st…
1bb5fd1 
…art command (fixes y-scope#1513). (y-scope#1545)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@LinZhihao-723 LinZhihao-723 LinZhihao-723 approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@junhaoliao @LinZhihao-723