fix pip check for optional dependencies #405
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis pull request introduces a new Python script within the CI support folder that processes the Changes
Sequence Diagram(s)sequenceDiagram
participant Runner as GitHub Action Runner
participant Conda as "Conda Config Step"
participant Mamba as "Setup Mambaforge"
participant Pip as "Pip Check Step"
participant Check as "check.py Script"
Runner->>Conda: Copy & append multiple environment files\n→ create environment.yml
Conda-->>Runner: environment.yml ready
Runner->>Mamba: Setup environment using environment.yml
Mamba-->>Runner: Environment configured
Runner->>Pip: Execute pip install with additional dependencies
Pip->>Check: Run `python .ci_support/check.py`
Check-->>Pip: Process and update pyproject.toml
Pip->>Runner: Display updated pyproject.toml contents
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #405 +/- ##
=======================================
Coverage 81.07% 81.07%
=======================================
Files 39 39
Lines 2309 2309
=======================================
Hits 1872 1872
Misses 437 437 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
.ci_support/check.py (1)
13-13: Consider using set operations for better efficiency.The current implementation first converts the list to a set and then back to a list. Using set operations directly would be more efficient.
Apply this diff to use set operations:
- data["project"]["dependencies"] += list(set(lst)) + existing_deps = set(data["project"]["dependencies"]) + optional_deps = set(lst) + data["project"]["dependencies"] = list(existing_deps | optional_deps)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.ci_support/check.py(1 hunks).ci_support/environment-qe.yml(1 hunks).github/workflows/pipeline.yml(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .ci_support/environment-qe.yml
🧰 Additional context used
🪛 Ruff (0.8.2)
.ci_support/check.py
5-5: Unnecessary open mode parameters
Remove open mode parameters
(UP015)
| with open("pyproject.toml", "r") as f: | ||
| data = tomlkit.load(f) |
There was a problem hiding this comment.
Add error handling for file operations and TOML parsing.
The script should handle potential file operation errors and TOML parsing exceptions to prevent crashes and data loss.
Apply this diff to add error handling:
- with open("pyproject.toml", "r") as f:
- data = tomlkit.load(f)
+ try:
+ with open("pyproject.toml") as f:
+ data = tomlkit.load(f)
+ except FileNotFoundError:
+ print("Error: pyproject.toml not found")
+ sys.exit(1)
+ except tomlkit.exceptions.TOMLKitError as e:
+ print(f"Error parsing pyproject.toml: {e}")
+ sys.exit(1)
- with open("pyproject.toml", "w") as f:
- f.writelines(tomlkit.dumps(data))
+ try:
+ with open("pyproject.toml", "w") as f:
+ f.writelines(tomlkit.dumps(data))
+ except IOError as e:
+ print(f"Error writing pyproject.toml: {e}")
+ sys.exit(1)Don't forget to add the required import:
import sysAlso applies to: 15-16
🧰 Tools
🪛 Ruff (0.8.2)
5-5: Unnecessary open mode parameters
Remove open mode parameters
(UP015)
.ci_support/check.py
Outdated
| lst = [] | ||
| for sub_lst in data["project"]["optional-dependencies"].values(): | ||
| for el in sub_lst: | ||
| lst.append(el) | ||
|
|
||
| data["project"]["dependencies"] += list(set(lst)) |
There was a problem hiding this comment.
Add validation for TOML structure and required fields.
The script assumes the presence of required fields in the TOML file without validation. This could lead to KeyError exceptions if the fields are missing.
Apply this diff to add validation:
- lst = []
- for sub_lst in data["project"]["optional-dependencies"].values():
- for el in sub_lst:
- lst.append(el)
+ lst = []
+ if "project" not in data:
+ print("Error: Missing 'project' section in pyproject.toml")
+ sys.exit(1)
+ if "optional-dependencies" not in data["project"]:
+ print("Error: Missing 'optional-dependencies' in project section")
+ sys.exit(1)
+ if "dependencies" not in data["project"]:
+ print("Error: Missing 'dependencies' in project section")
+ sys.exit(1)
+
+ for sub_lst in data["project"]["optional-dependencies"].values():
+ for el in sub_lst:
+ lst.append(el)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| lst = [] | |
| for sub_lst in data["project"]["optional-dependencies"].values(): | |
| for el in sub_lst: | |
| lst.append(el) | |
| data["project"]["dependencies"] += list(set(lst)) | |
| lst = [] | |
| if "project" not in data: | |
| print("Error: Missing 'project' section in pyproject.toml") | |
| sys.exit(1) | |
| if "optional-dependencies" not in data["project"]: | |
| print("Error: Missing 'optional-dependencies' in project section") | |
| sys.exit(1) | |
| if "dependencies" not in data["project"]: | |
| print("Error: Missing 'dependencies' in project section") | |
| sys.exit(1) | |
| for sub_lst in data["project"]["optional-dependencies"].values(): | |
| for el in sub_lst: | |
| lst.append(el) | |
| data["project"]["dependencies"] += list(set(lst)) |
.github/workflows/pipeline.yml
Outdated
| run: | | ||
| cp .ci_support/environment.yml environment.yml | ||
| tail --lines=+4 .ci_support/environment-lammps.yml >> environment.yml | ||
| tail --lines=+4 .ci_support/environment-qe.yml >> environment.yml | ||
| tail --lines=+4 .ci_support/environment-gpaw.yml >> environment.yml | ||
| echo -e "channels:\n - conda-forge\n" > .condarc |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for environment file existence.
The script assumes the existence of environment files without validation. This could lead to pipeline failures if files are missing.
Apply this diff to add validation:
run: |
- cp .ci_support/environment.yml environment.yml
- tail --lines=+4 .ci_support/environment-lammps.yml >> environment.yml
- tail --lines=+4 .ci_support/environment-qe.yml >> environment.yml
- tail --lines=+4 .ci_support/environment-gpaw.yml >> environment.yml
- echo -e "channels:\n - conda-forge\n" > .condarc
+ for file in \
+ ".ci_support/environment.yml" \
+ ".ci_support/environment-lammps.yml" \
+ ".ci_support/environment-qe.yml" \
+ ".ci_support/environment-gpaw.yml"; do
+ if [ ! -f "$file" ]; then
+ echo "Error: $file not found"
+ exit 1
+ fi
+ done
+
+ cp .ci_support/environment.yml environment.yml
+ for file in \
+ ".ci_support/environment-lammps.yml" \
+ ".ci_support/environment-qe.yml" \
+ ".ci_support/environment-gpaw.yml"; do
+ tail --lines=+4 "$file" >> environment.yml || {
+ echo "Error: Failed to append $file"
+ exit 1
+ }
+ done
+
+ echo -e "channels:\n - conda-forge\n" > .condarc📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| run: | | |
| cp .ci_support/environment.yml environment.yml | |
| tail --lines=+4 .ci_support/environment-lammps.yml >> environment.yml | |
| tail --lines=+4 .ci_support/environment-qe.yml >> environment.yml | |
| tail --lines=+4 .ci_support/environment-gpaw.yml >> environment.yml | |
| echo -e "channels:\n - conda-forge\n" > .condarc | |
| run: | | |
| for file in \ | |
| ".ci_support/environment.yml" \ | |
| ".ci_support/environment-lammps.yml" \ | |
| ".ci_support/environment-qe.yml" \ | |
| ".ci_support/environment-gpaw.yml"; do | |
| if [ ! -f "$file" ]; then | |
| echo "Error: $file not found" | |
| exit 1 | |
| fi | |
| done | |
| cp .ci_support/environment.yml environment.yml | |
| for file in \ | |
| ".ci_support/environment-lammps.yml" \ | |
| ".ci_support/environment-qe.yml" \ | |
| ".ci_support/environment-gpaw.yml"; do | |
| tail --lines=+4 "$file" >> environment.yml || { | |
| echo "Error: Failed to append $file" | |
| exit 1 | |
| } | |
| done | |
| echo -e "channels:\n - conda-forge\n" > .condarc |
| pip install versioneer[toml]==0.29 tomlkit | ||
| python .ci_support/check.py | ||
| cat pyproject.toml |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for script execution.
The script execution should be validated to ensure it completes successfully.
Apply this diff to add error handling:
- pip install versioneer[toml]==0.29 tomlkit
- python .ci_support/check.py
- cat pyproject.toml
+ pip install versioneer[toml]==0.29 tomlkit || {
+ echo "Error: Failed to install dependencies"
+ exit 1
+ }
+
+ if [ ! -f ".ci_support/check.py" ]; then
+ echo "Error: check.py not found"
+ exit 1
+ fi
+
+ python .ci_support/check.py || {
+ echo "Error: Failed to process dependencies"
+ exit 1
+ }
+
+ if [ ! -f "pyproject.toml" ]; then
+ echo "Error: pyproject.toml not found after processing"
+ exit 1
+ fi
+ cat pyproject.toml📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pip install versioneer[toml]==0.29 tomlkit | |
| python .ci_support/check.py | |
| cat pyproject.toml | |
| pip install versioneer[toml]==0.29 tomlkit || { | |
| echo "Error: Failed to install dependencies" | |
| exit 1 | |
| } | |
| if [ ! -f ".ci_support/check.py" ]; then | |
| echo "Error: check.py not found" | |
| exit 1 | |
| fi | |
| python .ci_support/check.py || { | |
| echo "Error: Failed to process dependencies" | |
| exit 1 | |
| } | |
| if [ ! -f "pyproject.toml" ]; then | |
| echo "Error: pyproject.toml not found after processing" | |
| exit 1 | |
| fi | |
| cat pyproject.toml |
Summary by CodeRabbit
pytorchas a new dependency in the environment configuration.