Try MP_API_KEY in Github Actions #332
Conversation
WalkthroughThe changes involve updates to the Continuous Integration (CI) workflow configuration in the Changes
Poem
Warning Rate limit exceeded@AntObi has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 13 minutes and 17 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. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
.github/workflows/ci.yml (1)
37-38: Consider documenting the required secretsIt would be helpful to document the required secrets in the repository's README or contributing guidelines to help other contributors set up their development environment correctly.
Consider adding a section like this to your README:
+ ## Required Secrets for CI + + The following secrets need to be configured in your repository settings: + + - `MP_API_KEY`: Materials Project API key for accessing their database + - `CODECOV_TOKEN`: Token for uploading coverage reports
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
.github/workflows/ci.yml(1 hunks)
🔇 Additional comments (1)
.github/workflows/ci.yml (1)
37-38: The environment variable configuration looks good!
The MP_API_KEY is correctly configured using GitHub's secure secrets mechanism, which is the recommended approach for handling sensitive credentials in CI workflows.
Let's verify if the Materials Project API integration is complete:
✅ Verification successful
The Materials Project API integration is properly implemented
The codebase shows a complete and well-structured integration of the MP_API_KEY:
- The key is correctly used in the download utility (
download_compounds_with_mp_api.py) with proper error handling - Test files appropriately check for the API key's presence using both environment variables and PyMatGen settings
- The implementation includes graceful fallbacks (checking both
MP_API_KEYandPMG_MAPI_KEY) - Tests are properly configured to skip when the API key is not available
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for Materials Project API usage in the codebase
# Expected: Find code that utilises the MP_API_KEY environment variable
# Search for Materials Project API imports and usage
rg -l "materials project|pymatgen"
# Search for environment variable usage
rg "MP_API_KEY|os\.environ\.get\(['\"]MP_API_KEY"
# Search for test files that might require the API key
fd -e py test_ -x rg -l "materials project|pymatgen|MP_API_KEY" {}
Length of output: 1980
Script:
#!/bin/bash
# Let's check the content of the files that use MP_API_KEY to understand the integration
cat smact/utils/crystal_space/download_compounds_with_mp_api.py
Length of output: 3571
Script:
#!/bin/bash
# Check if there are any configuration files that might handle the API key
rg -l "config|settings|api.*key" --type yaml --type ini --type toml
Length of output: 97
Script:
#!/bin/bash
# Check the test files to understand how the API key is used in tests
cat smact/tests/test_utils.py
Length of output: 7609
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #332 +/- ##
==========================================
+ Coverage 73.74% 76.42% +2.68%
==========================================
Files 31 31
Lines 2544 2545 +1
==========================================
+ Hits 1876 1945 +69
+ Misses 668 600 -68 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
pyproject.toml (1)
82-83: Consider documenting the rationale for kaleido dependencyThe addition of
kaleidoas a dependency should be documented to explain its purpose within the crystal_space feature set. This helps future maintainers understand the dependency's role.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
pyproject.toml(1 hunks)
🔇 Additional comments (1)
pyproject.toml (1)
82-83: Verify pydantic version constraint compatibility
The addition of pydantic<2.10.0 as a dependency warrants verification of compatibility with other packages. The strict version constraint might cause conflicts with other dependencies that require newer versions of pydantic.
✅ Verification successful
Pydantic version constraint is appropriate
The pydantic dependency is used minimally in the codebase, specifically in utilities.py for handling MP API responses. The version constraint pydantic<2.10.0 is appropriate because:
- The only usage is for type hints and data conversion of the MP API response
- No direct pydantic models or complex features are implemented
- The constraint helps maintain compatibility with the MP API client
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential dependency conflicts with pydantic
# Check if any other dependencies require pydantic
rg -l "pydantic" | grep -v "pyproject.toml"
# Check if mp-api has specific pydantic requirements
curl -s https://pypi.org/pypi/mp-api/json | jq '.info.requires_dist[]' | grep -i "pydantic"
Length of output: 173
Script:
#!/bin/bash
# Check the content of the file that uses pydantic
cat smact/structure_prediction/utilities.py
# Check if there are any other files importing pydantic
rg "from pydantic" -A 2
# Check if there are any other files using pydantic classes or models
rg "class.*\(.*pydantic" -A 2
Length of output: 2892
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
smact/tests/test_utils.py (2)
5-5: Consider migrating fully to pytestThe test file currently mixes unittest and pytest frameworks. While this works, it's generally better to stick to one testing framework for consistency and maintainability. Consider migrating the entire test suite to pytest, which offers more features and better readability.
Also applies to: 9-9
Line range hint
121-134: Use pytest's tmp_path fixtureInstead of manually managing temporary directories, consider using pytest's
tmp_pathfixture. This ensures proper cleanup and provides a unique temporary directory for each test run.Here's how you could refactor it:
- def test_download_compounds_with_mp_api(self): - save_mp_dir = "data/binary/mp_data" + def test_download_compounds_with_mp_api(self, tmp_path): + save_mp_dir = tmp_path / "binary" / "mp_data" download_compounds_with_mp_api.download_mp_data( mp_api_key=os.environ.get("MP_API_KEY"), num_elements=2, max_stoich=1, save_dir=save_mp_dir, ) # Check if the data was downloaded self.assertTrue(os.path.exists(save_mp_dir)) self.assertTrue(len(os.listdir(save_mp_dir)) > 0) - - # Clean up - shutil.rmtree(save_mp_dir)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
smact/tests/test_utils.py(2 hunks)
🔇 Additional comments (1)
smact/tests/test_utils.py (1)
116-116: Verify environment variable usage
The code accepts both MP_API_KEY and PMG_MAPI_KEY, but the test only uses MP_API_KEY. Let's verify the environment variable setup and usage.
Also applies to: 122-122
✅ Verification successful
Environment variable setup is correctly implemented
The environment variable usage is consistent across the codebase:
- The main functionality in
download_compounds_with_mp_api.pyrequiresMP_API_KEY - The test file appropriately handles both
MP_API_KEYand legacyPMG_MAPI_KEYfor backwards compatibility - GitHub Actions workflow correctly sets up
MP_API_KEYas a secret in the test job
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check environment variable usage and GitHub Actions setup
# Check for environment variable usage in the codebase
echo "Checking for MP_API_KEY usage:"
rg -l "MP_API_KEY"
echo -e "\nChecking for PMG_MAPI_KEY usage:"
rg -l "PMG_MAPI_KEY"
echo -e "\nChecking GitHub Actions workflow for MP_API_KEY:"
fd -t f "*.yml" .github/workflows/ --exec cat {} \; | grep -A 5 -B 5 "MP_API_KEY"
Length of output: 786
Script:
#!/bin/bash
# Let's examine the actual usage of these environment variables in the files
echo "Checking MP_API_KEY usage context in download_compounds_with_mp_api.py:"
rg -C 5 "MP_API_KEY" smact/utils/crystal_space/download_compounds_with_mp_api.py
echo -e "\nChecking both environment variables usage in test_utils.py:"
rg -C 5 "MP_API_KEY|PMG_MAPI_KEY" smact/tests/test_utils.py
echo -e "\nChecking GitHub Actions workflows with correct glob pattern:"
find .github/workflows -type f -name "*.yml" -exec cat {} \;
Length of output: 10055
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Test MP API KEY in the workflow
Description
To ensure that any future updates to SMACT don't break workflows dependent on the Materials Project API, I have added an API Key as a secret
Type of change
How Has This Been Tested?
Test Configuration:
Reviewers
N/A
Checklist
Summary by CodeRabbit
MP_API_KEYfor secure access to sensitive information during test execution in the CI workflow.crystal_spaceto includekaleidoandpydantic<2.10.0, enhancing functionality and data validation support.pytestfor improved test execution, including conditional skipping of tests based on the operating system and API key presence.