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.

Sign up for GitHub

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

Jump to bottom

OpSchema major rework #5740

Open
wants to merge 20 commits into
base: main
Choose a base branch
from
Open

OpSchema major rework #5740

wants to merge 20 commits into from
+967 −977

Conversation

mzient
Copy link
Contributor

@mzient mzient commented Dec 6, 2024 •
edited
Loading

Category:

Refactoring (OpSchema)
New feature (restricted seed argument)
Breaking change (kind of - only erroneous code is affected)

Description:

Visible effects:

  • seed is present only in schemas that need it
  • new OpSchema functions: AddRandomSeedArg and HasRandomSeedArg were added
  • OpSchema now allows for proper argument shadowing.
  • `OpSchema throws proper exceptions (not just DALIException everywhere)
    • This is visible from Python - different errors are raised now
  • Added invalid_key C++ exception, which is translated into Python KeyError

Performance:

  • query the arguments by string_view
  • cache parents
  • use a flattened set of arguments - don't traverse parent schemas when querying for default values

Code quality:

  • put all arguments in a single map: name->ArgumentDef
  • put all input info in a single array InputInfo (instead of separate devices, layouts, dox)
  • rewrite almost all argument handling - it's much simpler now

Safety:

  • add detection of circular inheritance

Additional information:

Affected modules and functionalities:

OpSchema
OpSpec
tests
random ops

Key points relevant for the review:

Tests:

  • Existing tests apply
  • New tests added
    • Python tests (new tests)
    • GTests (tests modified)
    • Benchmark
    • Other
  • N/A

Checklist

Documentation

  • Existing documentation applies
  • Documentation updated
    • Docstring (seed)
    • Doxygen (schema)
    • 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

@mzient mzient force-pushed the remove_implicit_seed branch from f2ff12f to 7fd4e73 Compare December 6, 2024 15:34
@dali-automaton
Copy link
Collaborator

CI MESSAGE: [21173981]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [21173981]: BUILD FAILED

mzient
mzient commented Dec 9, 2024
View reviewed changes
include/dali/core/error_handling.h
Comment on lines -90 to -91
const char *what() const noexcept override { return msg.c_str(); }
std::string msg;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This wasn't necessary at all.

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [21259855]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [21259855]: BUILD PASSED

mzient
mzient commented Dec 10, 2024
View reviewed changes
dali/pipeline/operator/op_schema_test.cc
Comment on lines +116 to +122
DALI_SCHEMA(Circular1)
.AddParent("Circular2");

DALI_SCHEMA(Circular2)
.AddParent("Circular1");

TEST(OpSchemaTest, CircularInheritance) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a new test - previously OpSchema would just get a stack overflow, now it throws (without a check it would hang).

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [21305575]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [21305575]: BUILD PASSED

1 similar comment
@dali-automaton
Copy link
Collaborator

CI MESSAGE: [21305575]: BUILD PASSED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [21339331]: BUILD STARTED

banasraf
banasraf reviewed Dec 11, 2024
View reviewed changes
dali/pipeline/operator/op_schema.h Outdated
*
* If the arg name starts is with an underscore, it will be marked hidden, which
* makes it not listed in the docs.
* If the arg name starts is with an underscore, it will be marked hidden, which
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
* If the arg name starts is with an underscore, it will be marked hidden, which
* If the arg name starts with an underscore, it will be marked hidden, which

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 (here and everywhere else).

stiepan
stiepan reviewed Dec 11, 2024
View reviewed changes
dali/pipeline/pipeline.cc Show resolved Hide resolved
dali/pipeline/operator/op_schema.cc Outdated Show resolved Hide resolved
dali/python/nvidia/dali/ops/__init__.py
Copy link
Member

Choose a reason for hiding this comment

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

Can we have a test for: emitting the warning on providing seed to non-randomized op and lack of the warning otherwise?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pfff.... I don't know how to test it (after all a warning is just printing something).

Copy link
Member

Choose a reason for hiding this comment

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

We even have assert_warns tool for that. :)

dali/pipeline/operator/op_schema.h Outdated Show resolved Hide resolved
mzient and others added 7 commits December 11, 2024 16:34
@mzient
Deprecate "seed" argument in the base schema.
4f49309 
Signed-off-by: Michał Zientkiewicz <[email protected]>
@mzient
Assign seed only to operators which require seed argument.
15ee470 
Signed-off-by: Michał Zientkiewicz <[email protected]>
@mzient
[WIP]
189493b 
Signed-off-by: Michał Zientkiewicz <[email protected]>
@mzient
OpSchema refactoring big time.
6370ed9 
Signed-off-by: Michal Zientkiewicz <[email protected]>
@mzient
Recactoring contd.
11b8e5e 
Signed-off-by: Michał Zientkiewicz <[email protected]>
@mzient
[WIP]
1995c35 
Signed-off-by: Michal Zientkiewicz <[email protected]>
@mzient
Milestone: dali_test compiles.
7085b1a 
Signed-off-by: Michal Zientkiewicz <[email protected]>
mzient and others added 12 commits December 11, 2024 16:34
@mzient
Native tests are (mostly) passing.
5e9c500 
Signed-off-by: Michal Zientkiewicz <[email protected]>
@mzient
Lint; remove dead code.
2315946 
Signed-off-by: Michal Zientkiewicz <[email protected]>
@mzient
Fix CheckArgs.
2714ff0 
Signed-off-by: Michal Zientkiewicz <[email protected]>
@mzient
Add missing AddRandomSeedArg.
786e20e 
Signed-off-by: Michal Zientkiewicz <[email protected]>
@mzient
Remove a buggy (?) error.
2512c97 
Signed-off-by: Michal Zientkiewicz <[email protected]>
@mzient
Add circular inheritance detection.
b1b1c58 
Signed-off-by: Michal Zientkiewicz <[email protected]>
@mzient
Add native key error.
daaceca 
Signed-off-by: Michał Zientkiewicz <[email protected]>
@mzient
Restore default value for seed for the sake of tests.
f4e256e 
Add more missing AddRandomSeedArg.
Change seed in resize tests (now it got "unlucky" one).

Signed-off-by: Michał Zientkiewicz <[email protected]>
@mzient
Document invalid_key class.
d719c48 
Signed-off-by: Michał Zientkiewicz <[email protected]>
@mzient
Hide the deprecated seed argument.
93cf6db 
Signed-off-by: Michał Zientkiewicz <[email protected]>
@mzient
Fix exceptions.
dd431d8 
Signed-off-by: Michal Zientkiewicz <[email protected]>
@mzient
Remove unused functionality. Fix typos in comments. string_view->stri…
b4106d8 
…ng move.

Signed-off-by: Michal Zientkiewicz <[email protected]>
@mzient mzient force-pushed the remove_implicit_seed branch from f4f2400 to b4106d8 Compare December 11, 2024 15:44
@dali-automaton
Copy link
Collaborator

CI MESSAGE: [21346246]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [21347293]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [21339331]: BUILD PASSED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [21347293]: BUILD PASSED

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@banasraf banasraf banasraf approved these changes

@stiepan stiepan stiepan approved these changes

Assignees

@stiepan stiepan

@banasraf banasraf

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@mzient @dali-automaton @stiepan @banasraf