-
Notifications
You must be signed in to change notification settings - Fork 21
rocAL Deserialize PR 2 : Initialize Arguments for Nodes #432
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
base: develop
Are you sure you want to change the base?
Conversation
Remove the use of pipeline operator
…ocAL into fg/ser_argument
Co-authored-by: spolifroni-amd <Sandra.Polifroni@amd.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
@fiona-gladwin Can you resolve merge conflicts? Thanks! |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #432 +/- ##
===========================================
- Coverage 79.97% 79.66% -0.31%
===========================================
Files 294 294
Lines 22560 22647 +87
===========================================
Hits 18041 18041
- Misses 4519 4606 +87
🚀 New features to boost your workflow:
|
rrawther
left 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.
Please address the review comments
| reader_cfg.set_sharding_info(sharding_info); | ||
|
|
||
| std::array<std::string, 23> arg_names = { | ||
| std::array<std::string, INIT_ARGS_COUNT> arg_names = { |
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 define as const std::string arg_array[] = {...}
and use size(arguments) to get size if needed. This is not recommended
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.
Since we are using std::array we have used the INIT_ARGS_COUNT to create the static array
|
|
||
| #include "pipeline/exception.h" | ||
|
|
||
| #define INIT_ARGS_COUNT 23 // Modify in accordance with number of args in 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.
remove hardcoded parameter. see comment below
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.
@rrawther The INIT_ARGS_COUNT is used to ensure that the number of args serialized is retrieved correctly during deserialize.
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 suggest adding a vector "std::vectorstd::string arguments" as part of the class and just copy the passed vector into the class member.
After that, you can just check the size of vector to get the size instead of hardcoded value
| } | ||
|
|
||
| void ImageLoaderNode::initialize_args(std::vector<Argument> &arguments, std::shared_ptr<MetaDataReader> meta_data_reader) { | ||
| if (arguments.size() != INIT_ARGS_COUNT) |
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.
use arg_array.size() instead
| reader_cfg.set_sharding_info(sharding_info); | ||
|
|
||
| std::array<std::string, 23> arg_names = { | ||
| std::array<std::string, INIT_ARGS_COUNT> arg_names = { |
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 see the comment above
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.
@rrawther The INIT_ARGS_COUNT is used to ensure that the number of args serialized is retrieved correctly during deserialize.
As in the initialize_args we need to check the required number of arguments are present to extract each of them and pass to the init function, If not it might lead to segmentation fault, hence we have hardcoded the value
| DecoderType decoder_type, bool shuffle, bool loop, size_t load_batch_count, RocalMemType mem_type, std::shared_ptr<MetaDataReader> meta_data_reader, bool decoder_keep_orig = false, const ShardingInfo& sharding_info = ShardingInfo(), | ||
| const char *prefix = "", unsigned sequence_length = 0, unsigned step = 0, unsigned stride = 0, ExternalSourceFileMode external_file_mode = ExternalSourceFileMode::NONE, const std::string &index_path = ""); | ||
|
|
||
| void initialize_args(std::vector<Argument> &arguments, std::shared_ptr<MetaDataReader> meta_data_reader) override; |
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 pass as const if the std::vector is read-only
rrawther
left 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.
Added review comments
|
|
||
| #include "pipeline/exception.h" | ||
|
|
||
| #define INIT_ARGS_COUNT 23 // Modify in accordance with number of args in 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.
I suggest adding a vector "std::vectorstd::string arguments" as part of the class and just copy the passed vector into the class member.
After that, you can just check the size of vector to get the size instead of hardcoded value
| ShardingInfo sharding_info(arguments[13].get<RocalBatchPolicy>(), arguments[14].get<bool>(), arguments[15].get<bool>(), arguments[16].get<int32_t>()); | ||
| std::string file_prefix = arguments[17].get<std::string>(); | ||
|
|
||
| this->init(arguments[0].get<unsigned>(), arguments[1].get<unsigned>(), arguments[2].get<std::string>(), |
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 get rid of hardcoded arguments in Init(), I suggest adding a new Init() function which takes the passed vector of arguments directly. Also you need to have some way of checking the values against expected. If so it will fail the Init 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.
@rrawther As suggested I have removed the hardcoded value and introduced a map instead where the arguments are obtained from the map using the name of the argument (i.e string) Please let me know if this works
Store the args in a map in the Node Modify initializers to accept and obtain values from the ArgumentSet using the string
Motivation
Introduce support to extract argument extraction and initialize the Node during deserialization.
Technical Details
Test Plan
No new tests added. Must pass existing tests
NOTE : To be merged after PR #430