Skip to content

Conversation

@ranareehanaslam
Copy link

The key changes made in the updated installation script~:

  1. Refactoring and Simplification: The updated script has been refactored for better readability and maintainability. Functions like get_installed_version and install_package are introduced to reduce code repetition and make the script more modular.

  2. Version Checks and Updates:

    • The tensorrt package version has been updated from 9.0.1.post11.dev4 to 9.3.0.post12.dev1. This change indicates a significant version update, possibly including new features, bug fixes, and performance improvements.
    • The CUDA Deep Neural Network library (nvidia-cudnn-cu11) dependency has been replaced with nvidia-cudnn-cu12 in the updated script, suggesting a move to support newer CUDA versions (cu12 instead of cu11). However, there seems to be a discrepancy in the version handling (8.9.7.29 vs. 8.9.6.50) that might be a typo or an intentional downgrade for compatibility reasons.
  3. Improved Package Management:

    • The updated script includes more sophisticated logic for managing package installations, including handling uninstallation of previous versions if needed, before installing a new version. This is especially evident in the handling of the tensorrt and nvidia-cudnn-cu12 packages.
    • The addition of no_cache_dir=True to all install_package calls to ensure the latest versions are fetched and to avoid potential conflicts with cached packages.
  4. Simplified Dependency Handling:

    • The dependencies for ONNX Graph Surgeon and Polygraphy are handled more straightforwardly in the updated script, with specific versions for dependent packages like protobuf.
  5. Error Handling and Robustness: The introduction of the get_installed_version function allows for more robust error handling by checking if a package is installed and obtaining its version before attempting to install or uninstall. This reduces the risk of errors during the installation process.

The key changes made in the updated installation script~:

1. **Refactoring and Simplification**: The updated script has been refactored for better readability and maintainability. Functions like `get_installed_version` and `install_package` are introduced to reduce code repetition and make the script more modular.

2. **Version Checks and Updates**:
   - The `tensorrt` package version has been updated from `9.0.1.post11.dev4` to `9.3.0.post12.dev1`. This change indicates a significant version update, possibly including new features, bug fixes, and performance improvements.
   - The CUDA Deep Neural Network library (`nvidia-cudnn-cu11`) dependency has been replaced with `nvidia-cudnn-cu12` in the updated script, suggesting a move to support newer CUDA versions (`cu12` instead of `cu11`). However, there seems to be a discrepancy in the version handling (`8.9.7.29` vs. `8.9.6.50`) that might be a typo or an intentional downgrade for compatibility reasons.

3. **Improved Package Management**:
   - The updated script includes more sophisticated logic for managing package installations, including handling uninstallation of previous versions if needed, before installing a new version. This is especially evident in the handling of the `tensorrt` and `nvidia-cudnn-cu12` packages.
   - The addition of `no_cache_dir=True` to all `install_package` calls to ensure the latest versions are fetched and to avoid potential conflicts with cached packages.

4. **Simplified Dependency Handling**:
   - The dependencies for ONNX Graph Surgeon and Polygraphy are handled more straightforwardly in the updated script, with specific versions for dependent packages like `protobuf`.

5. **Error Handling and Robustness**: The introduction of the `get_installed_version` function allows for more robust error handling by checking if a package is installed and obtaining its version before attempting to install or uninstall. This reduces the risk of errors during the installation process.
Copy link
Author

@ranareehanaslam ranareehanaslam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nvidia-cudnn-cu12 check version match 8.9.6.50

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might reccomend reconsidering the get-intsalled-version function defaults (no_cache_dir gets passed to the function with true rather than being default) Otherwise good code.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Against whitespace changes

Copy link

@ice-fly ice-fly left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Othewise excellent pull request, reccomend approval

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.

2 participants