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

Correct nvimagecodec version in conda and in installation instruction message #5714

Merged
merged 3 commits into from
Nov 18, 2024

Conversation

jantonguirao
Copy link
Contributor

Category:

Bug fix

Description:

Additional information:

Affected modules and functionalities:

Key points relevant for the review:

Tests:

  • Existing tests apply
  • New tests added
    • Python tests
    • GTests
    • Benchmark
    • Other
  • N/A

Checklist

Documentation

  • Existing documentation applies
  • Documentation updated
    • Docstring
    • Doxygen
    • RST
    • Jupyter
    • Other
  • N/A

DALI team only

Requirements

  • Implements new requirements
  • Affects existing requirements
  • N/A

REQ IDs: N/A

JIRA TASK: N/A

Signed-off-by: Joaquin Anton Guirao <[email protected]>
@jantonguirao
Copy link
Contributor Author

!build

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [20404509]: BUILD STARTED

@JanuszL JanuszL self-assigned this Nov 14, 2024
@stiepan stiepan self-assigned this Nov 14, 2024
Copy link
Member

Choose a reason for hiding this comment

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

Here there's the ${CUDA_MAJOR_VERSION} in the error message too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [20404509]: BUILD PASSED

Signed-off-by: Joaquin Anton Guirao <[email protected]>
@@ -149,9 +148,6 @@ Supported formats: JPEG, JPEG 2000, TIFF, PNG, BMP, PNM, PPM, PGM, PBM, WebP.
The output of the decoder is in *HWC* layout.

The implementation uses NVIDIA nvImageCodec to decode images.
You need to install it separately. See https://developer.nvidia.com/nvimgcodec-downloads
Copy link
Contributor Author

Choose a reason for hiding this comment

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

it is pulled as a dependency automatically, so no need for this sentence.

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [20431946]: BUILD STARTED

Copy link
Member

@stiepan stiepan left a comment

Choose a reason for hiding this comment

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

Looks good!

I got a rather generic nitpick that the build version included in package versioning is fully recognized by pypi, so what the message tells the user now is not 100% accurate, but maybe good enough. And hopefully, with the explicit dependency it is not a message we expect people to see often.

DALI requires nvImageCodec at minimum version 0.3.0, but got 0.2.0. Please upgrade: See https://developer.nvidia.com/nvimgcodec-downloads or simply do `pip install nvidia-nvimgcodec-cu12==0.3.0`.
Current pipeline object is no longer valid.

but

>>> pip install nvidia-nvimgcodec-cu12==0.3.0
ERROR: Could not find a version that satisfies the requirement nvidia-nvimgcodec-cu12==0.3.0 (from versions: 0.2.0.7, 0.3.0.5, 0.4.0.9)
ERROR: No matching distribution found for nvidia-nvimgcodec-cu12==0.3.0

@JanuszL
Copy link
Contributor

JanuszL commented Nov 15, 2024

Looks good!

I got a rather generic nitpick that the build version included in package versioning is fully recognized by pypi, so what the message tells the user now is not 100% accurate, but maybe good enough. And hopefully, with the explicit dependency it is not a message we expect people to see often.

DALI requires nvImageCodec at minimum version 0.3.0, but got 0.2.0. Please upgrade: See https://developer.nvidia.com/nvimgcodec-downloads or simply do `pip install nvidia-nvimgcodec-cu12==0.3.0`.
Current pipeline object is no longer valid.

but

>>> pip install nvidia-nvimgcodec-cu12==0.3.0
ERROR: Could not find a version that satisfies the requirement nvidia-nvimgcodec-cu12==0.3.0 (from versions: 0.2.0.7, 0.3.0.5, 0.4.0.9)
ERROR: No matching distribution found for nvidia-nvimgcodec-cu12==0.3.0

Maybe it should be pip install nvidia-nvimgcodec-cu12>=0.3.0 ?

@stiepan
Copy link
Member

stiepan commented Nov 15, 2024

Looks good!
I got a rather generic nitpick that the build version included in package versioning is fully recognized by pypi, so what the message tells the user now is not 100% accurate, but maybe good enough. And hopefully, with the explicit dependency it is not a message we expect people to see often.

DALI requires nvImageCodec at minimum version 0.3.0, but got 0.2.0. Please upgrade: See https://developer.nvidia.com/nvimgcodec-downloads or simply do `pip install nvidia-nvimgcodec-cu12==0.3.0`.
Current pipeline object is no longer valid.

but

>>> pip install nvidia-nvimgcodec-cu12==0.3.0
ERROR: Could not find a version that satisfies the requirement nvidia-nvimgcodec-cu12==0.3.0 (from versions: 0.2.0.7, 0.3.0.5, 0.4.0.9)
ERROR: No matching distribution found for nvidia-nvimgcodec-cu12==0.3.0

Maybe it should be pip install nvidia-nvimgcodec-cu12>=0.3.0 ?

I don't think so, that would install the most recent version (AFAIK pip does take into account installed packages requirements at this point), which may be incompatible.

What we probably could do is to include the patch version there, I think it is available in the header consumed at the built time.

@stiepan
Copy link
Member

stiepan commented Nov 15, 2024

Oh but wait, there's "compatible version" equality! pip install nvidia-nvimgcodec-cu12~=0.3.0

https://packaging.python.org/en/latest/specifications/version-specifiers/#compatible-release
https://docs.conda.io/projects/conda-build/en/latest/resources/package-spec.html#package-match-specifications

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [20431946]: BUILD PASSED

@jantonguirao jantonguirao merged commit 7fd3876 into NVIDIA:main Nov 18, 2024
5 of 6 checks passed
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.

4 participants