-
-
Notifications
You must be signed in to change notification settings - Fork 3
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
Dev #100
Conversation
Reviewer's Guide by SourceryThis pull request upgrades the project environment to PyTorch 2.3.1 with CUDA 11.8 support, updates dependencies, enhances the environment configuration, and adjusts the CodeQL CI workflow. The changes primarily focus on modernizing the development environment and improving the project's infrastructure. File-Level Changes
Tips
|
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.
Hey @ZhiboRao - I've reviewed your changes - here's some feedback:
Overall Comments:
- The PR description lacks detail. Please provide more context about the reasons for these changes and any testing performed to ensure compatibility.
- The update from PyTorch 1.7.1 to 2.3.1 is significant. Have you checked for any breaking changes that might affect the existing codebase?
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
@sourcery-ai review |
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.
Hey @ZhiboRao - I've reviewed your changes - here's some feedback:
Overall Comments:
- This is a significant update to the project environment. Please ensure comprehensive testing has been done to verify all functionality works correctly with the new versions, especially given the major version jump in PyTorch.
- The addition of Django to the dependencies suggests new web-related functionality. Consider documenting this new feature or capability in the README if it's intended for general use.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟡 Documentation: 3 issues found
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
@@ -38,7 +37,7 @@ This framework is only used in GPUs. | |||
**1) Build env** | |||
``` | |||
$ conda env create -f environment.yml |
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.
suggestion (documentation): Update PyTorch version references throughout the document
The PyTorch version has been updated from 1.7.1 to 2.3.1. Ensure this change is reflected consistently throughout the documentation, including any version-specific instructions or requirements.
| valImgNum | [int] | the number of images for val | 200 | | ||
| modelName | [str] | the model's name | NLCA-Net | | ||
| dataset | [str] | the dataset's name | SceneFlow | | ||
| web_cmd | [str] | cmd for django | 'main.py runserver 0.0.0.0:8000' | |
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.
suggestion (documentation): Provide more context for the new 'web_cmd' argument
Consider adding a brief explanation of when and how this argument is used, particularly in relation to the new Web/ directory.
| web_cmd | [str] | Django command for running the web server | 'main.py runserver 0.0.0.0:8000' |
| web_cmd | [str] | Used to start the Django server in the Web/ directory | 'main.py runserver 0.0.0.0:8000' |
@@ -102,6 +102,7 @@ | |||
| | ├── SysBasic/ | |||
| | ├── UserTemplate/ | |||
| | ├── FileHandler/ | |||
| | ├── Web/ |
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.
suggestion (documentation): Provide brief explanation for the new Web/ directory
Consider adding a short description of the purpose and contents of this new directory in the project structure section.
| | ├── Web/ | |
| | ├── Web/ # Handles web-related functionality and interfaces |
fixed some conflicts.
Summary by Sourcery
Update the project dependencies to use newer versions of PyTorch and CUDA, adjust the environment configuration accordingly, and update the README to reflect these changes. Modify the CI workflow to trigger on the 'dev' branch.
Enhancements:
CI: