Update pyproject.toml to resolve pip install issue #657
Conversation
Signed-off-by: Zishen Wang <[email protected]>
Signed-off-by: Zishen Wang <[email protected]>
Update _m3gnet.py
Solve GAP.json FileNotFoundError in pip install Signed-off-by: Zishen Wang <[email protected]>
|
Warning Rate limit exceeded@zz11ss11zz has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 16 minutes and 34 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe changes in this pull request involve updates to the Changes
Possibly related PRs
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
Documentation and Community
|
![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.yaml
Review profile: CHILL
Files selected for processing (2)
- pyproject.toml (1 hunks)
- src/maml/describers/_m3gnet.py (1 hunks)
Additional comments not posted (1)
src/maml/describers/_m3gnet.py (1)
137-137: Verify the intended behavior forself.return_type.The change from
isinstance(self.return_type, dict)toself.return_type is dictalters the logic from a type-checking approach to a direct identity comparison. This may lead to different behavior ifself.return_typeis allowed to be a subclass ofdict.Please confirm if
self.return_typeis strictly intended to be of typedictor if it can be a subclass ofdict.If
self.return_typeis allowed to be a subclass ofdict, then consider usingisinstance(self.return_type, dict)to maintain the original behavior.
| @@ -67,6 +67,7 @@ where = ["src"] | |||
| "describers/data/*.json", | |||
| "describers/data/megnet_models/*", | |||
| "describers/data/m3gnet_models/matbench_mp_e_form/0/m3gnet/*", | |||
| "apps/pes/params/*.json", | |||
There was a problem hiding this comment.
Tip
Codebase Verification
Update required: Incorrect path specified in pyproject.toml
The path "apps/pes/params/.json" specified in pyproject.toml doesn't match the actual location of the JSON files in the repository. The correct path appears to be "src/maml/apps/pes/params/.json". Please update the pyproject.toml file with the correct path to ensure that the necessary JSON files are included in the package distribution.
- Update line 70 in pyproject.toml to:
"src/maml/apps/pes/params/*.json",
On a positive note, no sensitive information was detected in any of the JSON files found in the repository.
Analysis chain
LGTM! This change resolves the reported installation issue.
The addition of "apps/pes/params/*.json" to the package-data section ensures that the necessary JSON files are included in the package distribution when installing with pip install. This should resolve the FileNotFoundError that was occurring.
Please verify that the JSON files in the apps/pes/params directory do not contain any sensitive information that should not be distributed with the package.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify JSON files in apps/pes/params do not contain sensitive information.
# Test: Search for common sensitive information patterns in the JSON files.
# Expect: No matches found.
rg --type json -i -e 'password|secret|token|key|credentials' apps/pes/params
Length of output: 134
Script:
#!/bin/bash
# Description: Locate JSON files and check for sensitive information
# Step 1: Find all JSON files in the repository
echo "Searching for JSON files:"
fd -e json
# Step 2: If JSON files are found, search for sensitive information patterns
echo -e "\nChecking JSON files for sensitive information:"
fd -e json -x rg -i -e 'password|secret|token|key|credentials' {}
# Step 3: Check if the specific directory mentioned in pyproject.toml exists
echo -e "\nChecking for the existence of apps/pes/params directory:"
fd -t d "apps/pes/params"
Length of output: 4518
Signed-off-by: Zishen Wang <[email protected]>
Signed-off-by: Zishen Wang <[email protected]>
Signed-off-by: Zishen Wang <[email protected]>
Signed-off-by: Zishen Wang <[email protected]>
Signed-off-by: Zishen Wang <[email protected]>
Signed-off-by: Zishen Wang <[email protected]>
Signed-off-by: Zishen Wang <[email protected]>
Signed-off-by: Zishen Wang <[email protected]>
Signed-off-by: Zishen Wang <[email protected]>
Signed-off-by: Zishen Wang <[email protected]>
Signed-off-by: Zishen Wang <[email protected]>
Signed-off-by: Zishen Wang <[email protected]>
Signed-off-by: Zishen Wang <[email protected]>
Signed-off-by: Zishen Wang <[email protected]>
Signed-off-by: Zishen Wang <[email protected]>
Signed-off-by: Zishen Wang <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 3
Outside diff range and nitpick comments (1)
pyproject.toml (1)
129-129: Consider enabling the ternary operator rule for concise and readable code.The change ignores the rule
"SIM108"that suggests using a ternary operator instead of anif-elseblock. Ternary operators can make the code more concise and readable when used appropriately for simple conditions.Ignoring this rule may lead to missed opportunities for simplifying and improving the code readability. It may result in unnecessary verbosity and less concise code in certain situations.
Please consider enabling the
"SIM108"rule and use ternary operators judiciously for simple conditions to improve code conciseness and readability:- "SIM108", # Use ternary operator instead of `if`-`else`-block
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (7)
- notebooks/direct/Example1_MPF2021.2.8.all.ipynb (1 hunks)
- notebooks/direct/Example2_Ti-H.ipynb (1 hunks)
- notebooks/gbe/GB energy model.ipynb (2 hunks)
- notebooks/model/garnet_formation_energy.ipynb (1 hunks)
- notebooks/pes/data/process_vasprun_to_mliap.ipynb (1 hunks)
- pyproject.toml (3 hunks)
- src/maml/apps/pes/_gap.py (1 hunks)
Files skipped from review due to trivial changes (6)
- notebooks/direct/Example1_MPF2021.2.8.all.ipynb
- notebooks/direct/Example2_Ti-H.ipynb
- notebooks/gbe/GB energy model.ipynb
- notebooks/model/garnet_formation_energy.ipynb
- notebooks/pes/data/process_vasprun_to_mliap.ipynb
- src/maml/apps/pes/_gap.py
| ] | ||
|
|
||
| [tool.black] | ||
| line-length = 120 | ||
|
|
||
| [tool.ruff] | ||
| target-version = "py311" | ||
| line-length = 120 | ||
| line-length = 160 |
There was a problem hiding this comment.
Consider adhering to the PEP 8 recommended line length limit.
The change increases the maximum allowed line length for the ruff linter from 120 to 160 characters. However, the Python style guide PEP 8 recommends a maximum line length of 79 characters for better code readability and maintainability.
Allowing longer lines can make the code harder to read, maintain, and may lead to inconsistencies in code formatting across the project.
Please consider reverting the change and adhering to the PEP 8 recommended line length limit of 79 characters:
-line-length = 160
+line-length = 79Committable 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.
| line-length = 160 | |
| line-length = 79 |
| @@ -112,6 +113,7 @@ | |||
| "B028", # No explicit stacklevel keyword argument found | |||
| "B904", # Within an except clause, raise exceptions with ... | |||
| "C408", # unnecessary-collection-call | |||
| "D103", # docstring is missing | |||
There was a problem hiding this comment.
Ensure all public functions have proper docstrings.
The change ignores the pydocstyle error "D103" for missing docstrings in public functions. However, docstrings provide important documentation for functions and help in understanding their purpose and usage.
Ignoring the missing docstring error may lead to undocumented or poorly documented code, making it harder for other developers to understand and maintain the codebase. It may also lead to confusion and errors when using the functions without proper documentation.
Please consider removing "D103" from the lint.ignore list and ensure that all public functions have proper docstrings:
- "D103", # docstring is missingCommittable 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.
| "D103", # docstring is missing |
| @@ -121,8 +123,10 @@ | |||
| "PLR", # pylint refactor | |||
| "PLW2901", # Outer for loop variable overwritten by inner assignment target | |||
| "PT013", # pytest-incorrect-pytest-import | |||
| "RET504", # assignment before `return` statement | |||
There was a problem hiding this comment.
Avoid assignments before return statements.
The change ignores the ruff rule "RET504" that checks for assignments before return statements. However, assignments before return statements can be confusing and may indicate a potential bug or unintended behavior.
Ignoring this rule may lead to less readable and maintainable code. It can make the code harder to understand and reason about, and may introduce subtle bugs or unintended behavior that can be difficult to detect and debug.
Please consider removing "RET504" from the lint.ignore list and refactor any code that violates this rule for better code quality and maintainability:
- "RET504", # assignment before `return` statementCommittable 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.
| "RET504", # assignment before `return` statement |
Signed-off-by: Zishen Wang <[email protected]>
Signed-off-by: Zishen Wang <[email protected]>
Signed-off-by: Zishen Wang <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
requirements-ci.txt (1)
4-4: Pinningmypyversion is a good practice for CI consistency.Specifying a fixed version for
mypyensures consistency across CI builds and aligns with the project's type checking requirements. This is a good practice for maintaining reproducibility and stability in CI environments.Consider applying a similar versioning strategy for other critical dependencies in this file to further enhance the stability and reproducibility of the CI pipeline.
Additionally, it's recommended to periodically review and update the pinned versions to benefit from bug fixes, performance improvements, and new features in the dependencies. You can use tools like Dependabot to automate the process of keeping the dependencies up to date.
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (3)
- requirements-ci.txt (1 hunks)
- requirements-dl.txt (1 hunks)
- requirements.txt (1 hunks)
Additional comments not posted (2)
requirements-dl.txt (1)
1-1: Review the impact of downgrading TensorFlow version.The TensorFlow version has been downgraded from 2.14.0 to 2.9.1. This change could potentially introduce compatibility issues if the codebase relies on features, optimizations, or bug fixes introduced in versions after 2.9.1.
Please review the codebase thoroughly to ensure that it does not depend on any TensorFlow features introduced after version 2.9.1. Pay special attention to any TensorFlow-related code changes made recently.
To assist with the review, you can run the following script to search for usage of TensorFlow features introduced after version 2.9.1:
This script searches for occurrences of "tensorflow" or "tf." in Python files, excluding version specifiers that match 2.9.1, and prints the matching lines with 5 lines of context. Review the output to identify any potential incompatibilities.
requirements.txt (1)
1-1: Clarify the reason for downgradingpymatgenand assess the potential impact.The version of
pymatgenhas been downgraded from2024.9.17.1to2023.10.11. This change may have significant implications for the project, especially if the codebase relies on features, bug fixes, or improvements introduced in the newer version.Please provide a clear justification for this downgrade and assess its potential impact on the project's functionality, performance, and compatibility.
Consider running the following script to identify any usage of
pymatgenfeatures that may be affected by the downgrade:
Signed-off-by: Zishen Wang <[email protected]>
Signed-off-by: Zishen Wang <[email protected]>
Signed-off-by: Zishen Wang <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (3)
- .github/workflows/testing.yml (1 hunks)
- pyproject.toml (3 hunks)
- src/maml/utils/_material.py (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- pyproject.toml
Additional context used
Ruff
src/maml/utils/_material.py
21-21: Use
X | Yfor type annotations(UP007)
Additional comments not posted (2)
src/maml/utils/_material.py (2)
5-5: LGTM!The import statement is necessary for the type annotation change in the
to_compositionfunction.
21-21: Excellent type annotation change!The change from using the bitwise OR operator (
|) to usingUnionfrom thetypingmodule enhances code clarity and adheres to best practices in type hinting. It makes the function's intent more understandable and potentially improves type checking during static analysis.Regarding the Ruff suggestion to use
X | Yfor type annotations, it appears to be a false positive in this case. The change to usingUnionis an improvement over the previous type annotation.Tools
Ruff
21-21: Use
X | Yfor type annotations(UP007)
| @@ -22,7 +22,7 @@ jobs: | |||
| max-parallel: 20 | |||
| matrix: | |||
| os: [ubuntu-latest] | |||
| python-version: ["3.10", "3.11"] | |||
| python-version: ["3.10", "3.9"] | |||
There was a problem hiding this comment.
Consider testing against Python 3.11
I noticed that the Python version in the testing matrix has been changed from ["3.10", "3.11"] to ["3.10", "3.9"]. Could you please clarify the reason for this change?
If Python 3.11 is being used in the production environment, it's important to include it in the testing matrix to ensure compatibility and catch any version-specific issues.
If there are no compatibility issues with the dependencies or other parts of the system, I recommend adding Python 3.11 back to the testing matrix. This will help ensure forward compatibility and allow the project to take advantage of new features and performance improvements in the latest Python version.
Signed-off-by: Zishen Wang <[email protected]>
Signed-off-by: Zishen Wang <[email protected]>
Signed-off-by: Zishen Wang <[email protected]>
Signed-off-by: Zishen Wang <[email protected]>
Signed-off-by: Zishen Wang <[email protected]>
Signed-off-by: Zishen Wang <[email protected]>
Signed-off-by: Zishen Wang <[email protected]>
Signed-off-by: Zishen Wang <[email protected]>
Signed-off-by: Zishen Wang <[email protected]>
Signed-off-by: Zishen Wang <[email protected]>
Signed-off-by: Zishen Wang <[email protected]>
Signed-off-by: Zishen Wang <[email protected]>
Signed-off-by: Zishen Wang <[email protected]>
Signed-off-by: Zishen Wang <[email protected]>
Signed-off-by: Zishen Wang <[email protected]>
Signed-off-by: Zishen Wang <[email protected]>
Signed-off-by: Zishen Wang <[email protected]>
Signed-off-by: Zishen Wang <[email protected]>
Signed-off-by: Zishen Wang <[email protected]>
Signed-off-by: Zishen Wang <[email protected]>
Signed-off-by: Zishen Wang <[email protected]>
Summary
maml/apps/pes/params/GAP.jsonis missing from the installed package usingpip installwith the errorFileNotFoundError. This update resolves this issue.Major changes:
Todos
If this is work in progress, what else needs to be done?
Checklist
ruff.mypy.duecredit@due.dcitedecorators to reference relevant papers by DOI (example)Tip: Install
pre-commithooks to auto-check types and linting before every commit: