-
Notifications
You must be signed in to change notification settings - Fork 557
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Freeze psutil/tensorflow dep #4995
Conversation
WalkthroughThe changes in this pull request involve modifications to two requirement files: Changes
Possibly related PRs
Suggested reviewers
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
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
requirements/github.txt (2)
Line range hint
4-11
: Consider freezing all dependency versionsTo ensure build stability (which is the PR's objective), consider pinning versions for all dependencies. Currently, some packages have version constraints while others (tensorflow-datasets, torch, torchvision) don't have any version specifications.
Consider updating the remaining dependencies with specific versions:
numpy<2 pydicom>=2.2.0 shapely>=1.7.1 tensorflow==2.15.0 -tensorflow-datasets -torch -torchvision +tensorflow-datasets==4.9.3 +torch==2.1.2 +torchvision==0.16.2
Dependencies from common.txt and test.txt are not consolidated in github.txt
The github.txt file still references common.txt and test.txt while only adding a few additional dependencies (numpy, pydicom, shapely, tensorflow, torch, torchvision). Most dependencies from common.txt (like aiofiles, boto3, httpx, etc.) and test.txt (like pytest and its plugins) are not present in github.txt.
- The following dependencies need to be consolidated:
- All dependencies from common.txt (30+ packages including httpx, pandas, matplotlib)
- All testing dependencies from test.txt (pytest and related packages)
- Current references to common.txt and test.txt should be removed after consolidation
🔗 Analysis chain
Line range hint
1-2
: Verify dependency coverage after removing requirement filesThe removal of
common.txt
andtest.txt
references changes the dependency management structure. Please ensure all necessary dependencies have been properly consolidated into this file.
Let me verify the contents of github.txt to understand what dependencies have been consolidated there.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check contents of removed requirement files echo "Checking for removed requirement files content:" for file in common.txt test.txt; do if [ -f "requirements/$file" ]; then echo "Content of $file:" cat "requirements/$file" fi done # Look for any other requirement files that might need consolidation echo -e "\nOther requirement files in the directory:" fd -e txt . requirements/Length of output: 1429
Script:
#!/bin/bash # Check contents of github.txt echo "Content of github.txt:" cat requirements/github.txt # Check if any dependencies from common.txt and test.txt are now in github.txt echo -e "\nChecking for dependencies from common.txt and test.txt in github.txt:" for dep in $(cat requirements/common.txt requirements/test.txt | cut -d'=' -f1); do echo "Checking $dep..." grep -i "^$dep[=><]" requirements/github.txt || true doneLength of output: 8770
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- requirements/github.txt (1 hunks)
- requirements/test.txt (1 hunks)
🔇 Additional comments (1)
requirements/test.txt (1)
Line range hint
1-11
: Verify TensorFlow compatibility with other dependenciesLet's check if other packages in the requirements have specific TensorFlow version constraints.
@@ -4,7 +4,7 @@ | |||
numpy<2 | |||
pydicom>=2.2.0 | |||
shapely>=1.7.1 | |||
tensorflow | |||
tensorflow==2.17.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix incorrect TensorFlow version number
The specified version tensorflow==2.17.0
appears to be incorrect, as TensorFlow's latest release is 2.15.0. This will cause package installation to fail.
-tensorflow==2.17.0
+tensorflow==2.15.0
📝 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.
tensorflow==2.17.0 | |
tensorflow==2.15.0 |
@@ -7,5 +7,5 @@ pytest-cov==4.0.0 | |||
pytest-mock==3.10.0 | |||
pytest-asyncio | |||
shapely | |||
tensorflow | |||
tensorflow==2.17.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Invalid TensorFlow version specified
The version 2.17.0
of TensorFlow does not exist. The latest stable version is 2.15.0 (as of April 2024). This will cause package installation failures.
Please verify and update to a valid TensorFlow version. Common choices are:
-tensorflow==2.17.0
+tensorflow==2.15.0 # Latest stable version
# or
+tensorflow==2.7.0 # If you need an older version
Committable suggestion was skipped due to low confidence.
What changes are proposed in this pull request?
An attempt to try and freeze testing dependencies so testing images will successfully build
See the previous build error aiming to fix
How is this patch tested? If it is not, please explain why.
Through watching if tests build successfully
Release Notes
Is this a user-facing change that should be mentioned in the release notes?
notes for FiftyOne users.
(Details in 1-2 sentences. You can just refer to another PR with a description
if this PR is part of a larger change.)
What areas of FiftyOne does this PR affect?
fiftyone
Python library changesSummary by CodeRabbit
Bug Fixes
tensorflow
package version to2.17.0
in both the main and test requirements, ensuring compatibility and functionality improvements.Chores
common.txt
andtest.txt
from the requirements.