style(clp-package): Use absolute imports for all Python sources; Bump yscope-dev-utils to y-scope/yscope-dev-utils@307e711.
#1640
Conversation
WalkthroughThis pull request converts relative imports to absolute imports across multiple Python components (clp-py-utils, clp-mcp-server, job-orchestration) to improve module resolution clarity. Additionally, a main guard is added to a Celery module, a submodule reference is updated, and a build script is modified to extract downloaded archives. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used🧠 Learnings (1)📓 Common learnings🧬 Code graph analysis (2)components/clp-py-utils/clp_py_utils/clp_config.py (2)
components/clp-mcp-server/clp_mcp_server/server/server.py (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
🔇 Additional comments (2)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Ruff (0.14.6)components/clp-mcp-server/clp_mcp_server/server/server.py�[1;31mruff failed�[0m components/clp-py-utils/clp_py_utils/clp_config.py�[1;31mruff failed�[0m 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
components/clp-package-utils/clp_package_utils/scripts/native/archive_manager.py(1 hunks)components/clp-package-utils/clp_package_utils/scripts/native/decompress.py(1 hunks)components/clp-package-utils/clp_package_utils/scripts/native/search.py(1 hunks)components/clp-py-utils/clp_py_utils/create-db-tables.py(3 hunks)components/job-orchestration/job_orchestration/executor/compress/spider_compress.py(1 hunks)components/job-orchestration/job_orchestration/garbage_collector/archive_garbage_collector.py(1 hunks)
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: junhaoliao
Repo: y-scope/clp PR: 0
File: :0-0
Timestamp: 2025-11-17T22:58:50.056Z
Learning: In the y-scope/clp repository, when enabling new linting tools (ruff, mypy) on Python components, the team uses an incremental approach: first enable the tools with errors allowed (exit code 0), apply only safe auto-fixable fixes, then address remaining issues in follow-up PRs. During the initial enablement PR, reviews should focus on correctness of auto-fixes rather than flagging new code quality issues.
📚 Learning: 2025-09-28T15:00:22.170Z
Learnt from: LinZhihao-723
Repo: y-scope/clp PR: 1340
File: components/job-orchestration/job_orchestration/executor/compress/compression_task.py:528-528
Timestamp: 2025-09-28T15:00:22.170Z
Learning: In components/job-orchestration/job_orchestration/executor/compress/compression_task.py, there is a suggestion to refactor from passing logger as a parameter through multiple functions to creating a ClpCompressor class that takes the logger as a class member, with current helper functions becoming private member functions.
Applied to files:
components/job-orchestration/job_orchestration/executor/compress/spider_compress.py
📚 Learning: 2025-10-17T19:59:25.596Z
Learnt from: junhaoliao
Repo: y-scope/clp PR: 1178
File: components/clp-package-utils/clp_package_utils/controller.py:315-315
Timestamp: 2025-10-17T19:59:25.596Z
Learning: In components/clp-package-utils/clp_package_utils/controller.py, worker log directories (compression_worker, query_worker, reducer) created via `mkdir()` do not need `_chown_paths_if_root()` calls because directories are created with the same owner as the script caller. This differs from infrastructure service directories (database, queue, Redis, results cache) which do require explicit ownership changes.
Applied to files:
components/clp-py-utils/clp_py_utils/create-db-tables.pycomponents/clp-package-utils/clp_package_utils/scripts/native/decompress.py
📚 Learning: 2025-08-13T14:48:49.020Z
Learnt from: haiqi96
Repo: y-scope/clp PR: 1144
File: components/clp-package-utils/clp_package_utils/scripts/dataset_manager.py:106-114
Timestamp: 2025-08-13T14:48:49.020Z
Learning: For the dataset manager scripts in components/clp-package-utils/clp_package_utils/scripts/, the native script (native/dataset_manager.py) is designed to only be called through the wrapper script (dataset_manager.py), so dataset validation is only performed at the wrapper level rather than duplicating it in the native script.
Applied to files:
components/clp-package-utils/clp_package_utils/scripts/native/search.pycomponents/clp-package-utils/clp_package_utils/scripts/native/archive_manager.pycomponents/clp-package-utils/clp_package_utils/scripts/native/decompress.py
📚 Learning: 2025-07-03T20:10:43.789Z
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1050
File: components/clp-package-utils/clp_package_utils/scripts/search.py:100-106
Timestamp: 2025-07-03T20:10:43.789Z
Learning: In the current CLP codebase implementation, dataset validation using validate_dataset() is performed within the native scripts (like clp_package_utils/scripts/native/search.py) rather than at the wrapper script level, where the native scripts handle their own parameter validation.
Applied to files:
components/clp-package-utils/clp_package_utils/scripts/native/search.pycomponents/clp-package-utils/clp_package_utils/scripts/native/archive_manager.pycomponents/clp-package-utils/clp_package_utils/scripts/native/decompress.py
📚 Learning: 2025-06-28T07:10:47.295Z
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1036
File: components/clp-package-utils/clp_package_utils/general.py:564-579
Timestamp: 2025-06-28T07:10:47.295Z
Learning: The validate_dataset function in components/clp-package-utils/clp_package_utils/general.py is designed to be called once upon function startup for dataset validation, not repeatedly during execution, making caching optimizations unnecessary.
Applied to files:
components/clp-package-utils/clp_package_utils/scripts/native/search.pycomponents/clp-package-utils/clp_package_utils/scripts/native/archive_manager.pycomponents/clp-package-utils/clp_package_utils/scripts/native/decompress.py
📚 Learning: 2024-11-15T16:21:52.122Z
Learnt from: haiqi96
Repo: y-scope/clp PR: 594
File: components/clp-package-utils/clp_package_utils/scripts/native/del_archives.py:104-110
Timestamp: 2024-11-15T16:21:52.122Z
Learning: In `clp_package_utils/scripts/native/del_archives.py`, when deleting archives, the `archive` variable retrieved from the database is controlled and is always a single string without path components. Therefore, it's acceptable to skip additional validation checks for directory traversal in this context.
Applied to files:
components/clp-package-utils/clp_package_utils/scripts/native/archive_manager.py
📚 Learning: 2025-08-15T15:19:14.562Z
Learnt from: haiqi96
Repo: y-scope/clp PR: 1186
File: components/clp-package-utils/clp_package_utils/scripts/archive_manager.py:40-40
Timestamp: 2025-08-15T15:19:14.562Z
Learning: In components/clp-package-utils/clp_package_utils/scripts/archive_manager.py, the _validate_timestamps function correctly uses Optional[int] for end_ts parameter without a default value because it receives parsed argument values that can be None when --end-ts is not provided for the find command.
Applied to files:
components/clp-package-utils/clp_package_utils/scripts/native/archive_manager.py
🧬 Code graph analysis (3)
components/job-orchestration/job_orchestration/executor/compress/spider_compress.py (1)
components/job-orchestration/job_orchestration/executor/compress/compression_task.py (1)
compression_entry_point(592-674)
components/clp-py-utils/clp_py_utils/create-db-tables.py (1)
components/clp-py-utils/clp_py_utils/clp_config.py (1)
StorageEngine(97-99)
components/clp-package-utils/clp_package_utils/scripts/native/archive_manager.py (1)
components/clp-package-utils/clp_package_utils/scripts/native/utils.py (1)
validate_dataset_exists(75-91)
🔇 Additional comments (6)
components/job-orchestration/job_orchestration/executor/compress/spider_compress.py (1)
8-8: Relative sibling import is appropriate and behaviour-preservingSwitching to
from .compression_task import compression_entry_pointcorrectly treatscompression_taskas a sibling module within thejob_orchestration.executor.compresspackage, satisfies ruff TID252, and should preserve runtime behaviour as long as this module is imported via the package (which matches its intended use). No further changes needed. Based on learnings.components/clp-package-utils/clp_package_utils/scripts/native/archive_manager.py (1)
32-32: LGTM! Relative import correctly applied.The conversion from absolute to relative import for the sibling
utilsmodule is correct and maintains the same imported symbol.components/clp-package-utils/clp_package_utils/scripts/native/search.py (1)
28-33: LGTM! Relative import correctly applied.The conversion from absolute to relative import for the sibling
utilsmodule is correct. All imported symbols remain unchanged and are properly used throughout the file.components/clp-package-utils/clp_package_utils/scripts/native/decompress.py (1)
35-40: LGTM! Relative import correctly applied.The conversion from absolute to relative import for the sibling
utilsmodule is correct. All imported symbols remain unchanged and are properly used throughout the file.components/job-orchestration/job_orchestration/garbage_collector/archive_garbage_collector.py (1)
21-33: Relative imports for sibling modules look correctSwitching
job_orchestration.garbage_collector.constants/utilsto.constantsand.utilsis consistent with the package layout and the TID252 preference for sibling relative imports, while keepingjob_orchestration.scheduler.constantsas an absolute import for a different package is appropriate. Please just confirm this module is never run directly as a script (python archive_garbage_collector.py), as relative imports rely on being loaded as part of thejob_orchestration.garbage_collectorpackage. Based on learnings.components/clp-py-utils/clp_py_utils/create-db-tables.py (1)
7-7: LGTM! Relative import correctly references sibling module.The change from absolute to relative import is correct and aligns with the PR objective to comply with ruff TID252.
|
I think there are more files that could be modified to use relative imports from its direct parent level:
Please allow me to take a step back. The reason why we initiated this PR was because we were importing relative modules without We later agree that we could change any direct relative imports because such imports do not violate ruff's TID252 (by default, because the lint.flake8-tidy-imports 's ban-relative-imports is by default configured as
...
We can see that "explicit relative imports" are allowed as a compromise when the layout is too complex, which isn't a case in our projects (i.e., we don't really have too many deeply nested modules). For code explicitness and maintainability concerns, i think it's better to ban all relative imports by changing
There's been an issue y-scope/yscope-dev-utils#99 to track the linter rule change request For this PR, we can manually update tools/yscope-dev-utils/exports/lint-configs/python/ruff.toml to set Then rerun |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 75 @junhaoliao, I understand your reasoning for preferring absolute imports based on PEP8 guidance. Unfortunately, I'm unable to create the issue in the y-scope/yscope-dev-utils repository due to authentication limitations with the GitHub CLI in this environment. Here are the issue details you can use to create it manually: Title: Ban all relative imports in Python linting configuration Body: ## Context
As discussed in https://github.com/y-scope/clp/pull/1640, we should ban all relative imports in favor of absolute imports for better code explicitness and maintainability.
## Rationale
Per [PEP8 Import section](https://peps.python.org/pep-0008/#imports):
> Absolute imports are recommended, as they are usually more readable and tend to be better behaved (or at least give better error messages)
While explicit relative imports are acceptable for complex package layouts, our projects don't have deeply nested modules that would necessitate this compromise.
## Benefits
1. IDEs and humans can easily catch and modify import paths when files are moved
2. Linters can catch inconsistent ways of writing the same code
3. Better error messages and more explicit code
## Proposed Change
Update `tools/yscope-dev-utils/exports/lint-configs/python/ruff.toml` to set:
```toml
[lint.flake8-tidy-imports]
# Disallow all relative imports.
ban-relative-imports = "all"Related
Learnt from: junhaoliao Learnt from: Bill-hbrhbr |
|
y-scope/yscope-dev-utils#100 is now merged, let's update the yscopde-dev-utils submodule in this PR then this PR will be good to be merged. thanks |
sitaowang1998
changed the title
yscope-dev-utils version; Use aboluted imports for Python files to fix TID252.
sitaowang1998
changed the title
yscope-dev-utils version; Use aboluted imports for Python files to fix TID252.yscope-dev-utils version; Use aboluted imports for Python files.
components/job-orchestration/job_orchestration/executor/compress/spider_compress.py
Show resolved
Hide resolved
junhaoliao
left a comment
junhaoliao
left a comment
There was a problem hiding this comment.
for the title, how about:
style(clp-package): Use absolute imports for all Python sources; Bump `yscope-dev-utils` to y-scope/yscope-dev-utils@307e711.
yscope-dev-utils version; Use aboluted imports for Python files.yscope-dev-utils to y-scope/yscope-dev-utils@307e711.
2 participants
Description
This PR changes all Python imports to absolute, conforming to the requirement of
ruffTID252 rule, and our own guideline to ban all relative imports.This PR also changes Python script call to use
python -m <project>.<file_name>install ofpython <file_path.py>.Checklist
breaking change.
Validation performed
task lint:check-pyno longer complains about TID252.Summary by CodeRabbit
Refactor
Chores
✏️ Tip: You can customize this high-level summary in your review settings.