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 an ability to return a duplicated outputs from the DALI pipeline #1556

Merged
merged 1 commit into from
Dec 17, 2019
Merged

Add an ability to return a duplicated outputs from the DALI pipeline #1556

merged 1 commit into from
Dec 17, 2019

Conversation

JanuszL
Copy link
Contributor

@JanuszL JanuszL commented Dec 9, 2019

  • adds a code that prevents adding "MakeContiguous" multiple times to the output
  • adds tests for CPU, GPU and Mixed operators outputs that duplicates
  • some small clean up in used API checker in the Pipeline

Signed-off-by: Janusz Lisiecki jlisiecki@nvidia.com

Why we need this PR?

  • add an ability to return a duplicated outputs from the DALI pipeline

What happened in this PR?

  • adds a code that prevents adding "MakeContiguous" multiple times to the output
  • adds a visible deprecation warning for some pipeline methods
  • some small code cleanup
  • tests are added for the duplicated outputs (CPU, GPU and Mixed operators outputs)
  • no docs update is necessary

JIRA TASK: [475]

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [1024038]: BUILD STARTED

Comment on lines 262 to 269
for name in outputs:
idx = output_map.get(name, next_free_idx + 1)
if idx > next_free_idx:
output_map[name] = next_free_idx
idx = next_free_idx
next_free_idx += 1
unique_outputs.append(name)
self._outputs_list.append(idx)
Copy link
Contributor

Choose a reason for hiding this comment

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

I wanted to suggest something that is clearer when indexing, but it does lookup the dictionary twice.

for name in outputs:
    if name not in output_map:
        output_map[name] = next_free_idx
        next_free_idx +=1
        unique_outputs.append(name)
     self._outputs_list.append(output_map[name])

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 not perf critical. Done.

pipeline_tls = tls()

def show_deprecation_warning(deprecated, in_favor_of):
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
def show_deprecation_warning(deprecated, in_favor_of):
def _show_deprecation_warning(deprecated, in_favor_of):

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

Copy link
Contributor

@klecki klecki left a comment

Choose a reason for hiding this comment

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

Minor nitpick, otherwise ok.

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [1024053]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [1024038]: BUILD FAILED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [1024053]: BUILD FAILED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [1024285]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [1024285]: BUILD FAILED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [1024302]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [1024302]: BUILD FAILED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [1024321]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [1024323]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [1024321]: BUILD FAILED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [1024321]: BUILD PASSED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [1024323]: BUILD PASSED

.AddOutput("contiguous_" + name, "cpu");
PrepareOpSpec(&spec, GetNextInternalLogicalId());

graph_.AddOp(spec, "__MakeContiguous_" + name);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
graph_.AddOp(spec, "__MakeContiguous_" + name);
graph_.AddOp(spec, "__MakeContiguous_" + name);
it->second.has_contiguous = true;

This should solve the issue without the need for an extra unordererd_set - and I think it's missing 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.

Done

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [1029963]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [1030120]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [1030120]: BUILD FAILED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [1029963]: BUILD FAILED

- adds a code that prevents adding "MakeContiguous" multiple times to
  the output
- adds tests for CPU, GPU and Mixed operators outputs that duplicates
- some small clean up in used API checker in the Pipeline

Signed-off-by: Janusz Lisiecki <jlisiecki@nvidia.com>
@JanuszL JanuszL requested a review from klecki December 12, 2019 18:38
@dali-automaton
Copy link
Collaborator

CI MESSAGE: [1030348]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [1030348]: BUILD PASSED

@JanuszL JanuszL merged commit 22ca969 into NVIDIA:master Dec 17, 2019
@JanuszL JanuszL deleted the allow_duplicated_outputs branch December 17, 2019 14:46
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.

None yet

5 participants