Skip to content
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

Merged
merged 6 commits into from
Sep 21, 2024
Merged

Dev #100

merged 6 commits into from
Sep 21, 2024

Conversation

ZhiboRao
Copy link
Member

@ZhiboRao ZhiboRao commented Sep 20, 2024

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:

  • Update the environment configuration to use PyTorch 2.3.1 and CUDA 11.8, along with other updated dependencies.
  • Modify the README to reflect the updated environment and add a new command for Django web server execution.
  • Add a new 'Web' directory to the project structure in the documentation.

CI:

  • Change the CodeQL workflow to trigger on the 'dev' branch instead of 'master'.

Copy link

sourcery-ai bot commented Sep 20, 2024

Reviewer's Guide by Sourcery

This 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

Change Details Files
Upgrade PyTorch and CUDA versions
  • Update PyTorch to version 2.3.1
  • Update CUDA to version 11.8
  • Update torchaudio and torchvision to compatible versions
environment.yml
Update and optimize dependencies
  • Remove outdated dependencies
  • Add new dependencies like Django
  • Update versions of existing dependencies
  • Reorganize dependency list for better readability
environment.yml
meta.yaml
Enhance environment configuration
  • Add new channels including NVIDIA and PyTorch mirror
  • Remove outdated or unnecessary channels
  • Update environment name to reflect new PyTorch version
environment.yml
Update README.md
  • Remove LGTM badge
  • Update conda environment activation command
  • Add 'web_cmd' argument to the arguments table
README.md
Modify CodeQL workflow
  • Change target branch for push events from 'master' to 'dev'
.github/workflows/codeql.yml

Tips
  • Trigger a new Sourcery review by commenting @sourcery-ai review on the pull request.
  • Continue your discussion with Sourcery by replying directly to review comments.
  • You can change your review settings at any time by accessing your dashboard:
    • Enable or disable the Sourcery-generated pull request summary or reviewer's guide;
    • Change the review language;
  • You can always contact us if you have any questions or feedback.

Copy link

@sourcery-ai sourcery-ai bot left a 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

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.

@ZhiboRao ZhiboRao merged commit 0aa194d into master Sep 21, 2024
5 checks passed
@ZhiboRao
Copy link
Member Author

@sourcery-ai review

Copy link

@sourcery-ai sourcery-ai bot left a 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

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
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
Copy link

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' |
Copy link

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/
Copy link

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.

Suggested change
| | ├── Web/
| | ├── Web/ # Handles web-related functionality and interfaces

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant