-
Notifications
You must be signed in to change notification settings - Fork 618
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
Pipeline decorator #2629
Pipeline decorator #2629
Conversation
Signed-off-by: Michał Szołucha <mszolucha@nvidia.com>
Signed-off-by: Michał Szołucha <mszolucha@nvidia.com>
Signed-off-by: Michał Szołucha <mszolucha@nvidia.com>
Signed-off-by: Michał Szołucha <mszolucha@nvidia.com>
bbfea0e
to
484d370
Compare
!build |
CI MESSAGE: [2018301]: BUILD STARTED |
CI MESSAGE: [2018301]: BUILD PASSED |
Signed-off-by: Michał Szołucha <mszolucha@nvidia.com>
Signed-off-by: Michał Szołucha <mszolucha@nvidia.com>
dali/python/nvidia/dali/pipeline.py
Outdated
@@ -990,3 +996,62 @@ def iter_setup(self): | |||
For example, one can use this function to feed the input | |||
data from NumPy arrays.""" | |||
pass | |||
|
|||
|
|||
def _discriminate_args(**kwargs): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
def _discriminate_args(**kwargs): | |
def _separate_args(**kwargs): |
or
def _discriminate_args(**kwargs): | |
def _split_args(**kwargs): |
dali/python/nvidia/dali/pipeline.py
Outdated
**Usage** | ||
|
||
First, implement a function, that defines a DALI pipeline. | ||
Such function shall return DALI's DataNodes. These return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Such function shall return DALI's DataNodes. These return | |
Such function shall return one or more DALI DataNodes, representing the outputs of the pipeline. |
dali/python/nvidia/dali/pipeline.py
Outdated
return ctor_args, fn_args | ||
|
||
|
||
def pipeline(fn=None, **pipeline_kwargs): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this name can be misleading because we have
nvidia.dali.pipeline.Pipeline
(the pipeline class) and nvidia.dali.pipeline.pipeline
the decorator.
How about renaming to pipeline_def
to avoid confusion?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I agree, pipeline_def
or def_pipeline
, or def_pipe
or make_pipeline
. Whatever to no have three different nvidia.dali.pipeline.[Pp]ipeline things.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally, I'm fine with pipeline
, because we do already have the Pipeline
in the same package and place. So we do already:
from nvidia.dali.pipeline import Pipeline
pipe = Pipeline(...)
Why not doing
from nvidia.dali.pipeline import pipeline
@pipeline
def pipe():
...
If you make a mistake there, the interpreter will boldly tell you about it
Let's leave more sophisticated naming for the user:
import nvidia.dali.pipeline.pipeline as pipeline_decorator
import nvidia.dali.pipeline.Pipeline as pipeline_object
dali/python/nvidia/dali/pipeline.py
Outdated
my_pipe = pipe(0, batch_size=32, num_threads=1, device_id=0, flip_horizontal=1) | ||
my_pipe.build() | ||
|
||
Additionally, decorator can accept Pipeline constructor |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Additionally, decorator can accept Pipeline constructor | |
Additionally, the decorator can accept Pipeline constructor |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
return pipeline | ||
|
||
|
||
@nottest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do you need nottest? The function won't be run automatically (not prefixed with test_). If you need that, why is it not there in reference_pipeline
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To test, that pipeline decorator works with other decorators
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not obvious, maybe add a comment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
@@ -20,7 +20,7 @@ | |||
|
|||
try: | |||
from nvidia.dali.plugin.pytorch import DALIClassificationIterator, LastBatchPolicy | |||
from nvidia.dali.pipeline import Pipeline | |||
from nvidia.dali.pipeline import pipeline |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
have we decided that we want to replace examples?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm replacing subset of examples
pipeline.set_outputs(images, labels) | ||
return pipeline | ||
|
||
@pipeline |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we decide to go ahead with changing the examples, I'd use here the whole name:
@nvidia.dali.pipeline.pipeline_def
or @nvidia.dali.pipeline.pipeline
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the examples we use Pipeline
instead of nvidia.dali.pipeline.Pipeline
, so I believe it's better to use pipeline_def
instead of nvidia.dali.pipeline.pipeline_def
@@ -14,77 +14,76 @@ | |||
|
|||
|
|||
import torch | |||
from nvidia.dali.pipeline import Pipeline | |||
from nvidia.dali.pipeline import pipeline |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as in the previous file, do we want to replace our examples to use this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm replacing subset of examples
Signed-off-by: Michał Szołucha <mszolucha@nvidia.com>
!build |
CI MESSAGE: [2033757]: BUILD STARTED |
pipeline.set_outputs(images, labels) | ||
return pipeline | ||
|
||
@pipeline |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having a more distinct name from the pipeline module and Pipeline object would remove the need for the full blown specification here.
dali/python/nvidia/dali/pipeline.py
Outdated
|
||
def pipeline(fn=None, **pipeline_kwargs): | ||
""" | ||
Decorator for wrapping functions that define DALI pipelines. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe add something like: Decorated functions becomes a pipeline factory
? Or Decorated function will return instances of Pipeline with processing defined by the function
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The latter - or sth like:
Decorator for wrapping functions that define DALI pipelines. | |
A decorator which creates a factory of DALI pipelines whose processing graph is defined by the decorated function. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
dali/python/nvidia/dali/pipeline.py
Outdated
_ = pipe_outputs if isinstance(pipe_outputs, tuple) else pipe_outputs, | ||
pipe.set_outputs(*_) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PLZ NO.
_
is for ignored values.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
CI MESSAGE: [2033757]: BUILD FAILED |
dali/python/nvidia/dali/pipeline.py
Outdated
**Usage** | ||
|
||
First, implement a function, that defines a DALI pipeline. | ||
Such function shall return DALI's DataNodes. These return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It can return not only DataNodes.
I'm missing the information, that this decorator will turn this function into something like Pipeline factory. And that the returns of this function become output of the Pieline (for example .run()) and the function will return Pipeline instances.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
dali/python/nvidia/dali/pipeline.py
Outdated
data = fn.external_source(source=my_generator) | ||
return data | ||
|
||
my_pipe = pipe(batch_size=128) # batch_size=128 overwrites batch_size=32 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm also thinking if the doc should mention that this is equivalent to using with pipe:
and set_outputs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
docs/pipeline.rst
Outdated
@@ -71,3 +84,9 @@ DataNode | |||
-------- | |||
.. autoclass:: DataNode | |||
:members: | |||
|
|||
|
|||
Decorator |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This subtitle is subpar.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suits me good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not good at all.
Something like Pipeline factor decorator
would be more appropriate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Decorator | |
Pipeline factory decorator |
or
Decorator | |
Pipeline definition decorator |
would be better ideas.
dali/python/nvidia/dali/pipeline.py
Outdated
""" | ||
Decorator for wrapping functions that define DALI pipelines. | ||
|
||
**Usage** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that some description should go before usage/example:
``pipeline_def`` returns a function that creates a pipeline, where the processing graph is defined by the function ``fn`` passed as the first positional argument. It can be used as a decorator, in which case the decorated function becomes a pipeline factory.
The function returned is roughly equivalent to::
def factory(arg1, arg2, ...)
pipe = nvidia.dali.Pipeline()
with pipe:
pipe.set_outputs(*fn(arg1, arg2, ...))
return pipe
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe that all this is written when you google "how to write a decorator?". So I wouldn't add it at the beginning, but I added it at the end of the paragraph, cause this note is in fact valuable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, but.... how can you possibly google that a decorator creates a DALI pipeline, calls your function within the with-scope of the newly created pipeline and calls set_outputs with the return values of the function you supplied???
Perhaps it should be written as text, not as code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the doc is fine, we shouldn't use multi compound statements, it is hard to follow.
I was thinking whether to move the equivalent code to the top, but I'm not sure if it's better than usage examples.
dali/python/nvidia/dali/pipeline.py
Outdated
def _discriminate_args(**kwargs): | ||
"""Split args on those applicable to Pipeline constructor and the rest.""" | ||
fca = inspect.getfullargspec(Pipeline.__init__) # Full Ctor Args | ||
ctor_args = dict(filter(lambda x: x[0] in fca.args or x[0] in fca.kwonlyargs, kwargs.items())) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be the other way round: detect arguments for this function - if not found, try Pipeline - if not found either, raise an error.
Rationale: if we add new arguments to Pipeline's constructor, we may break users' code because a named argument would be redirected to the Pipeline constructor instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done with one exception: there's be a problem with the error part. I believe we'd like this to be legal:
@pipeline_def
def peculiar_pipeline(operator=fn.old_color_twist, **operator_kwargs):
return operator(**operator_kwargs)
p = peculiar_pipeline(operator=fn.color_twist, brightness=5)
So the remaining args we still need to assign to the func_args
dali/python/nvidia/dali/pipeline.py
Outdated
pipe = Pipeline(**{**pipeline_kwargs, **ctor_args}) # Merge and overwrite dict | ||
with pipe: | ||
pipe_outputs = func(*args, **fn_kwargs) | ||
po = pipe_outputs if isinstance(pipe_outputs, tuple) else (pipe_outputs,) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
po = pipe_outputs if isinstance(pipe_outputs, tuple) else (pipe_outputs,) | |
po = pipe_outputs if isinstance(pipe_outputs, tuple) else () if pipe outputs is None else (pipe_outputs,) |
@@ -990,3 +996,93 @@ def iter_setup(self): | |||
For example, one can use this function to feed the input | |||
data from NumPy arrays.""" | |||
pass | |||
|
|||
|
|||
def _discriminate_args(func, **func_kwargs): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder how it plays with political correctness ;P
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mentioned it before but the comment got lost.
- I'd use "split" or "separate", discriminate might carry a negative connotation.
- There is a function
def _separate_kwargs(kwargs):
in ops.py. Isn't this exactly what you need? (haven't read it)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a discriminator in electronics, mathematics, GANs also have a discriminator. I wouldn't like to be oversensitive - should we avoid to use "gap", just because there is a wage-gap?
(sacrasm warning)
After all, that's precisely what this function does - treats function args better than ctor args ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Achievement unlocked: troll ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about the existing function I pointed out? Is it a duplicate or not?
dali/python/nvidia/dali/pipeline.py
Outdated
if farg[0] in func_argspec.args or farg[0] in func_argspec.kwonlyargs: | ||
fn_args[farg[0]] = farg[1] | ||
elif farg[0] in ctor_argspec.args or farg[0] in ctor_argspec.kwonlyargs: | ||
ctor_args[farg[0]] = farg[1] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about a warning when shadowing?
if farg[0] in func_argspec.args or farg[0] in func_argspec.kwonlyargs: | |
fn_args[farg[0]] = farg[1] | |
elif farg[0] in ctor_argspec.args or farg[0] in ctor_argspec.kwonlyargs: | |
ctor_args[farg[0]] = farg[1] | |
is_fn_arg = farg[0] in func_argspec.args or farg[0] in func_argspec.kwonlyargs | |
is_pipe_arg = farg[0] in ctor_argspec.args or farg[0] in ctor_argspec.kwonlyargs | |
if is_fn_arg: | |
if is_pipe_arg: | |
print("Warning: the argument `{}` shadows a Pipeline constructor argument of the same name.".format(farg[0])) | |
fn_args[farg[0]] = farg[1] | |
elif is_pipe_arg: | |
ctor_args[farg[0]] = farg[1] |
|
||
pipe = my_pipe(batch_size=128) # batch_size=128 overrides batch_size=32 | ||
|
||
.. warning:: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.. warning:: | |
.. warning:: | |
Avoid the use of `**kwargs` in the graph definition function, since it may result in unwanted, | |
silent hijacking of some arguments of the same name by Pipeline constructor. Code written | |
this way may cease to work with future versions of DALI when new parameters are added | |
to the Pipeline constructor. | |
.. warning:: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't we just disallow the **kwargs
completely for the function that is to be decorated and raise an error. I'd say play safe rather than write lengthy manuals. If we find a better way or this is an issue we can always relax that check (and doing the other way would be hard).
@@ -990,3 +996,93 @@ def iter_setup(self): | |||
For example, one can use this function to feed the input | |||
data from NumPy arrays.""" | |||
pass | |||
|
|||
|
|||
def _discriminate_args(func, **func_kwargs): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mentioned it before but the comment got lost.
- I'd use "split" or "separate", discriminate might carry a negative connotation.
- There is a function
def _separate_kwargs(kwargs):
in ops.py. Isn't this exactly what you need? (haven't read it)
return pipeline | ||
|
||
|
||
@nottest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not obvious, maybe add a comment?
|
||
|
||
|
||
@nottest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in other test suites we use check_
. You wouldn't have to mark it as nottest if you follow this suggestion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
check_
is vague. When you see it, it doesn't answer a question, why is there a function called in the same way, just check_
instead of test_
? And @nottest
gives you a clear answer ;)
yield test_pipeline_static, vert, hori | ||
yield test_pipeline_runtime, vert, hori | ||
yield test_pipeline_override, vert, hori, 16 | ||
yield test_pipeline_runtime, fn.random.coin_flip(seed=123), fn.random.coin_flip(seed=234) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this work? can we pass data nodes from outside with pipe:
... ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apparently yes...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can if they don't have side-effects.
General comment: Please use the template for the PR description (answering the not relevant questions with N/A if needed) |
Signed-off-by: Michał Szołucha <mszolucha@nvidia.com>
Signed-off-by: Michał Szołucha <mszolucha@nvidia.com>
"""Split args on those applicable to Pipeline constructor and the decorated function.""" | ||
func_argspec = inspect.getfullargspec(func) | ||
ctor_argspec = inspect.getfullargspec(Pipeline.__init__) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if func_argspec.varkw is not None: | |
raise ValueError("Use of variadic keyword argument `**{}` in graph definition function is not allowed.".format(func_argspec.varkw)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that this is probably the simplest and cleanest solution, that we can easily document - just say that all the variadic kwargs are used for forwarding arguments to the pipeline and the original function is not supposed to use it/have it as we may steal them with changes to Pipeline class.
dali/python/nvidia/dali/pipeline.py
Outdated
raise TypeError( | ||
"Using non-explicitly declared arguments in graph-defining function is not allowed. " | ||
"Please remove `{}` argument or declare it explicitly in the function signature.".format( | ||
farg[0])) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
raise TypeError( | |
"Using non-explicitly declared arguments in graph-defining function is not allowed. " | |
"Please remove `{}` argument or declare it explicitly in the function signature.".format( | |
farg[0])) | |
raise ValueError("`{}` is neither an argument of the graph definition function nor of `Pipeline.__init__`".format(farg[0])) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can explicitly check **kwargs (see above), but we can still produce a nice error here - but with a slightly different descirption.
dali/python/nvidia/dali/pipeline.py
Outdated
Pipeline constructor. Code written this way may cease to work with future versions of DALI | ||
when new parameters are added to the Pipeline constructor. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pipeline constructor. Code written this way may cease to work with future versions of DALI | |
when new parameters are added to the Pipeline constructor. | |
Pipeline constructor. Code written this way would cease to work with future versions of DALI | |
when new parameters are added to the Pipeline constructor. |
It's not allowed, so this explanation is describes a purely hypothetical scenario.
def test_kwargs_exception_1(): | ||
pipeline_kwargs(1, arg2=2) | ||
|
||
|
||
def test_kwargs_exception_2(): | ||
pipeline_kwonlyargs(1, arg2=2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These should raise an exception for mere presence of **kwargs.
dali/python/nvidia/dali/pipeline.py
Outdated
elif is_ctor_arg: | ||
ctor_args[farg[0]] = farg[1] | ||
else: | ||
fn_args[farg[0]] = farg[1] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not just raise here? If we disable kwargs that would save us from guessing as it would be an explicit error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would disallow the **kwargs
from the original signature, otherwise ok.
docs/pipeline.rst
Outdated
#. by inheriting from Pipeline class and overriding :meth:`Pipeline.define_graph` | ||
#. by instantiating `Pipeline` directly, building the graph and setting the pipeline | ||
outputs with :meth:`Pipeline.set_outputs` | ||
#. by implementing a function that uses DALI's ``Operators`` inside and decorating it with the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#. by implementing a function that uses DALI's ``Operators`` inside and decorating it with the | |
#. by implementing a function that uses DALI operators inside and decorating it with the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One nitpick, otherwise looks good.
fn_args[farg[0]] = farg[1] | ||
if is_ctor_arg: | ||
print( | ||
"Warning: the argument `{}` shadows a Pipeline constructor argument of the same name.".format( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider
"Warning: the argument `{}` shadows a Pipeline constructor argument of the same name.".format( | |
f"Warning: the argument `{farg[0]}` shadows a Pipeline constructor argument of the same name." |
elif is_ctor_arg: | ||
ctor_args[farg[0]] = farg[1] | ||
else: | ||
assert False, "This shouldn't happen. Please double-check the `{}` argument".format(farg[0]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider
assert False, "This shouldn't happen. Please double-check the `{}` argument".format(farg[0]) | |
raise RuntimeError(f"This shouldn't happen. Please double-check the `{farg[0]}` argument") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, except for some minor suggestions
!build |
CI MESSAGE: [2070571]: BUILD STARTED |
CI MESSAGE: [2070571]: BUILD PASSED |
Signed-off-by: Michał Szołucha mszolucha@nvidia.com
This PR adds new feature to the API: Pipeline decorator. It eases up pipeline creation and likely will help users, especially those who are new to DALI
Why we need this PR?
Pick one, remove the rest
What happened in this PR?
Fill relevant points, put NA otherwise. Replace anything inside []
added decorator
pipeline.py
NA
YEs
Yes
JIRA TASK: [Use DALI-XXXX or NA]