-
Notifications
You must be signed in to change notification settings - Fork 863
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
move dali preprocess to handler util #2485
Conversation
Signed-off-by: jagadeesh <jagadeeshj@ideas2it.com>
@lxning reg Feature #2332, is the expectation is to
|
Codecov Report
@@ Coverage Diff @@
## master #2485 +/- ##
==========================================
- Coverage 72.71% 70.87% -1.84%
==========================================
Files 79 83 +4
Lines 3742 3839 +97
Branches 58 58
==========================================
Hits 2721 2721
- Misses 1017 1114 +97
Partials 4 4
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more 📢 Have feedback on the report? Share it here. |
Signed-off-by: jagadeesh <jagadeeshj@ideas2it.com>
Signed-off-by: jagadeesh <jagadeeshj@ideas2it.com>
Signed-off-by: jagadeesh <jagadeeshj@ideas2it.com>
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.
Please add steps for generating the "default.dali' built-in pipeline. We will need the build scripts for this and actual Dali pipeline creation code
I think this looks reasonable, would really be much simpler to merge if we have a test though |
Signed-off-by: jagadeesh <jagadeeshj@ideas2it.com>
Signed-off-by: jagadeesh <jagadeeshj@ideas2it.com>
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.
@jagadeeshi2i
Could you please add a pytest for this feature.
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.
@jagadeeshi2i Looks good overall.
Couple of points
- Is there any reason for changing resnet to mnist
- The args in builtin dali_pipeline_generation.py should be configurable
- Can we re-use the pipeline in dali_pipeline_generation.py in the example.
The current example is defining a new pipeline?
Ex: for a simple image classifier model, users should be able to just use the default pipeline. Let me know your thoughts?
ts/handler_utils/preprocess/built-in/dali_pipeline_generation.py
Outdated
Show resolved
Hide resolved
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.
@jagadeeshi2i Please add the documentation for how to compile the built in handler pipeline. This will need to be integrated with the torch build steps.
- remove dali_image_segmenter handler Signed-off-by: jagadeesh <jagadeeshj@ideas2it.com>
Signed-off-by: jagadeesh <jagadeeshj@ideas2it.com>
Signed-off-by: jagadeesh <jagadeeshj@ideas2it.com>
This reverts commit 0fc8ba4.
Signed-off-by: jagadeesh <jagadeeshj@ideas2it.com>
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.
ResNet example works by manually copying default.dali to the ta/handle_utils/built_in folder where torchserve is installed.
Approving to unblock.
Description
Fixes #2332
Type of change
Please delete options that are not relevant.
Feature/Issue validation/testing
Please describe the Unit or Integration tests that you ran to verify your changes and relevant result summary. Provide instructions so it can be reproduced.
Please also list any relevant details for your test configuration.
Test A
Logs for Test A
Test B
Logs for Test B
Checklist: