Skip to content

Enable more Python types to be supported by the DALI python function #5598

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

Merged
merged 2 commits into from
Aug 9, 2024

Conversation

JanuszL
Copy link
Contributor

@JanuszL JanuszL commented Aug 6, 2024

  • adds support to DALI to and from DLPack conversion
  • extends types support for fn.ones, fn.full to include
    int8, uin32, uint64
  • adds tests

Category:

Other (e.g. Documentation, Tests, Configuration)

Description:

  • adds support to DALI to and from DLPack conversion
  • extends types support for fn.ones, fn.full to include
    int8, uin32, uint64
  • adds tests

Additional information:

Affected modules and functionalities:

  • DALI_TYPE_SWITCH_WITH_FP16
  • to and from DLPack conversion

Key points relevant for the review:

  • NA

Tests:

  • Existing tests apply
  • New tests added
    • Python tests
      • test_python_function.test_different_types
    • 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

@JanuszL
Copy link
Contributor Author

JanuszL commented Aug 6, 2024

!build

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [17252244]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [17252244]: BUILD FAILED

- adds support to DALI to and from DLPack conversion
- extends types support for fn.ones, fn.full to include
  int8, uin32, uint64
- adds tests

Signed-off-by: Janusz Lisiecki <[email protected]>
@JanuszL JanuszL force-pushed the enable_more_python_typoes branch from 0bf3570 to 5c24338 Compare August 6, 2024 17:11
@JanuszL
Copy link
Contributor Author

JanuszL commented Aug 6, 2024

!build

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [17256060]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [17256060]: BUILD PASSED

@@ -636,6 +636,12 @@ inline std::ostream &operator<<(std::ostream &os, DALIDataType dtype) {
{__VA_ARGS__} \
} \
break; \
case DALI_INT8: \
Copy link
Contributor

Choose a reason for hiding this comment

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

Food for thought:

This macro is used only once - I'd remove it, add some definition of tensor-compatible types and use TYPE_SWITCH instead.

#define DALI_INTEGRAL_TYPES uint8_t, int8_t, uint16_t, int16_t, uint32_t, int32_t, uint64_t, int64_t
#define DALI_NUMERIC_TYPES DALI_INTEGRAL_TYPES, float, double
#define DALI_NUMERIC_TYPES_FP16 DALI_NUMERIC_TYPES, float16

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Comment on lines 49 to 57
// Common types
using uint8 = uint8_t;
using uint16 = uint16_t;
using int16 = int16_t;
using int64 = int64_t;
using uint32 = uint32_t;
using uint64 = uint64_t;
using int8 = int8_t;
using int16 = int16_t;
using int32 = int32_t;
using uint32 = uint32_t;
using int64 = int64_t;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we remove those instead? They're not used that much and many places in the code use the standard names anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed all the types and converted all the occurrence to *_t.

Copy link
Contributor

@mzient mzient left a comment

Choose a reason for hiding this comment

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

Approved, but I'm not happy about extending proliferating vestigial features.

@JanuszL JanuszL force-pushed the enable_more_python_typoes branch 2 times, most recently from 124b92c to e71eac3 Compare August 7, 2024 19:13
@JanuszL
Copy link
Contributor Author

JanuszL commented Aug 7, 2024

!build

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [17295335]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [17295335]: BUILD FAILED

Signed-off-by: Janusz Lisiecki <[email protected]>
@JanuszL JanuszL force-pushed the enable_more_python_typoes branch from e71eac3 to 9fcf8fd Compare August 8, 2024 06:24
@JanuszL
Copy link
Contributor Author

JanuszL commented Aug 8, 2024

!build

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [17311969]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [17311969]: BUILD PASSED

@JanuszL JanuszL merged commit 7cdfa0e into NVIDIA:main Aug 9, 2024
7 checks passed
@JanuszL JanuszL deleted the enable_more_python_typoes branch August 9, 2024 14:04
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