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

Add C API call to check backend of an operator #3031

Merged
merged 5 commits into from
Jun 11, 2021

Conversation

banasraf
Copy link
Collaborator

@banasraf banasraf commented Jun 9, 2021

Why we need this PR?

  • It adds new feature needed in DALI Triton backend.

We need to check the backend of an ExternalSource in DALI Triton backend.

What happened in this PR?

  • What solution was applied:
    Added an C API call to check backend of an operator.
  • Affected modules and functionalities:
    c_api.h/cc
  • Key points relevant for the review:
    api design
  • Validation and testing:
    added case to C API tests
  • Documentation (including examples):
    API call documentation

JIRA TASK: DALI-2000

@banasraf banasraf changed the title Add a C API call to check backend of an operator Add C API call to check backend of an operator Jun 9, 2021
@banasraf banasraf force-pushed the add-checking-es-backend branch 2 times, most recently from d134464 to dd10c97 Compare June 9, 2021 14:10
@klecki klecki self-assigned this Jun 9, 2021
Signed-off-by: Rafal <Banas.Rafal97@gmail.com>
Signed-off-by: Rafal <Banas.Rafal97@gmail.com>
@szalpal szalpal self-assigned this Jun 10, 2021
@@ -543,6 +543,21 @@ void daliGetReaderMetadata(daliPipelineHandle* pipe_handle, const char *reader_n
meta->stick_to_shard = returned_meta.stick_to_shard;
}

dali_backend_t daliGetOperatorBackend(daliPipelineHandle* pipe_handle, const char *name) {
Copy link
Member

Choose a reason for hiding this comment

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

How about operator_name? Or op_name? Won't hurt us ;)

Copy link
Collaborator 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 421 to 422
DLL_PUBLIC dali_backend_t daliGetOperatorBackend(daliPipelineHandle* pipe_handle,
const char *name);
Copy link
Member

Choose a reason for hiding this comment

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

How about operator_name? Or op_name? Won't hurt us ;)

Copy link
Collaborator 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 45 to 49
typedef enum {
DALI_BACKEND_GPU = 0,
DALI_BACKEND_CPU = 1,
DALI_BACKEND_MIXED = 2
} dali_backend_t;
Copy link
Member

Choose a reason for hiding this comment

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

Couple things here.

In device_type_t we assign CPU=0 and GPU=1 and here it's reversed. How about making this consistent?

CPU=0, GPU=1, MIXED=2

Secondly, in device_type_t we don't use DALI prefix for the enum name. Maybe we should keep this consistent also? After all, there's dali already in type name, so it's still readable:

typedef enum {
  CPU = 0,
  GPU = 1,
  MIXED = 2
} dali_backend_t;

dali_backend_t::CPU;
dali_backend_t::GPU;
dali_backend_t::MIXED;

Copy link
Collaborator Author

@banasraf banasraf Jun 10, 2021

Choose a reason for hiding this comment

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

It's device type that is inconsistent with the rest of the enums. Also, C enums without a prefix are a bad idea, because you can reference them without device_type_t::*, so our namespace is polluted with CPU and GPU.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I can't even name those CPU and GPU. The names are already taken by the device_type_t

Copy link
Member

@szalpal szalpal Jun 10, 2021

Choose a reason for hiding this comment

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

It's device type that is inconsistent with the rest of the enums

I'm wondering, that in C API I see only device_type_t enum so far, so maybe it's good idea to keep them consistent within single API?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

About the reverse order - it's taken from OpType

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm wondering, that in C API I see only device_type_t enum so far, so maybe it's good idea to keep them consistent within single API?

typedef enum {
  DALI_NO_TYPE  = -1,
  DALI_UINT8    =  0,
  DALI_UINT16   =  1,
  DALI_UINT32   =  2,
  DALI_UINT64   =  3,
  DALI_INT8     =  4,
  DALI_INT16    =  5,
  DALI_INT32    =  6,
  DALI_INT64    =  7,
  DALI_FLOAT16  =  8,
  DALI_FLOAT    =  9,
  DALI_FLOAT64  =  10,
  DALI_BOOL     =  11,
} dali_data_type_t;

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I changed the order of the GPU and CPU

Rafal added 2 commits June 10, 2021 10:36
Signed-off-by: Rafal <Banas.Rafal97@gmail.com>
Signed-off-by: Rafal <Banas.Rafal97@gmail.com>
@banasraf
Copy link
Collaborator Author

!build

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [2460513]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [2460513]: BUILD FAILED

Signed-off-by: Rafal <Banas.Rafal97@gmail.com>
@banasraf
Copy link
Collaborator Author

!build

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [2460868]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [2460868]: BUILD PASSED

@banasraf banasraf merged commit 9bce8bd into NVIDIA:main Jun 11, 2021
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