[Scheduler][PIC] create pdl as list #1513
Open
aserhani wants to merge 15 commits intoShamrock-code:mainfrom
Open
[Scheduler][PIC] create pdl as list #1513aserhani wants to merge 15 commits intoShamrock-code:mainfrom
aserhani wants to merge 15 commits intoShamrock-code:mainfrom
Conversation
Summary of ChangesHello @aserhani, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a significant architectural change by transforming the single Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Contributor
|
Thanks @aserhani for opening this PR! You can do multiple things directly here: Once the workflow completes a message will appear displaying informations related to the run. Also the PR gets automatically reviewed by gemini, you can: |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the PatchDataLayerLayout from a single object to a std::deque, allowing for multiple layouts. The changes are propagated through ShamrockCtx, PatchScheduler, and related test files. The default layout is consistently set to the first element of the list, which aligns with the PR's goal of an incremental update.
My review has identified two main issues:
- A critical bug in
PatchScheduler.hppwhere a validation check constructs an exception but fails to throw it, rendering the check ineffective. - A high-severity bug in
pyShamrockCtx.cppthat could lead to data loss when collecting data for Python bindings due to overwriting dictionary keys for field names that appear in multiple layers.
I have provided code suggestions to fix both issues. The rest of the changes look good and are consistent with the stated objective.
Comment on lines
135
to
144
| for (u32 layer_idx = 0; layer_idx < pdl_ptr_list.size(); layer_idx++) { | ||
| if (!pdl(layer_idx).check_main_field_type<vectype>()) { | ||
| std::invalid_argument( | ||
| std::string("the main field is not of the correct type to call this function\n") | ||
| + "fct called : " + __PRETTY_FUNCTION__ | ||
| + shambase::format("current patch data layout of index {} : ", layer_idx) | ||
| + pdl(layer_idx).get_description_str() | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
This loop has a critical issue: std::invalid_argument is constructed but not thrown, which means the type check is ineffective. The program will continue even with a type mismatch, potentially leading to undefined behavior. You must throw the exception.
Additionally, for improved readability and to follow modern C++ practices, consider using a range-based for loop. To include the index in the error message, you can maintain a counter variable.
u32 layer_idx = 0;
for (const auto& pdl_ptr : pdl_ptr_list) {
if (!pdl_ptr->check_main_field_type<vectype>()) {
throw std::invalid_argument(
std::string("the main field is not of the correct type to call this function\n")
+ "fct called : " + __PRETTY_FUNCTION__
+ shambase::format("current patch data layout of index {} : ", layer_idx)
+ pdl_ptr->get_description_str()
);
}
layer_idx++;
}
src/shampylib/src/pyShamrockCtx.cpp
Outdated
Comment on lines
289
to
309
| for (u32 layer_idx = 0; layer_idx < ctx.pdl_list.size(); layer_idx++) { | ||
| for (auto fname : ctx.pdl_list.at(layer_idx)->get_field_names()) { | ||
| append_to_map<f32>(fname, data, dic_out); | ||
| append_to_map<f32_2>(fname, data, dic_out); | ||
| append_to_map<f32_3>(fname, data, dic_out); | ||
| append_to_map<f32_4>(fname, data, dic_out); | ||
| append_to_map<f32_8>(fname, data, dic_out); | ||
| append_to_map<f32_16>(fname, data, dic_out); | ||
| append_to_map<f64>(fname, data, dic_out); | ||
| append_to_map<f64_2>(fname, data, dic_out); | ||
| append_to_map<f64_3>(fname, data, dic_out); | ||
| append_to_map<f64_4>(fname, data, dic_out); | ||
| append_to_map<f64_8>(fname, data, dic_out); | ||
| append_to_map<f64_16>(fname, data, dic_out); | ||
| append_to_map<u32>(fname, data, dic_out); | ||
| append_to_map<u64>(fname, data, dic_out); | ||
| append_to_map<u32_3>(fname, data, dic_out); | ||
| append_to_map<u64_3>(fname, data, dic_out); | ||
| append_to_map<i64_3>(fname, data, dic_out); | ||
| } | ||
| } |
There was a problem hiding this comment.
The current implementation iterates through each layer and then through each field name, calling append_to_map for each. If the same field name exists in multiple layers, the data for that field in the output dictionary (dic_out) will be overwritten by subsequent calls to append_to_map. This can lead to data loss.
To fix this, you should first collect all unique field names from all layers and then iterate over this unique set to populate the dictionary. This ensures that each field is processed only once, aggregating data correctly across all patches as append_to_map does.
Additionally, using range-based for loops would make the code more modern and readable. Note that you may need to include the <set> header for std::set.
std::set<std::string> all_field_names;
for (const auto& pdl_ptr : ctx.pdl_list) {
for (const auto& fname : pdl_ptr->get_field_names()) {
all_field_names.insert(fname);
}
}
for (const auto& fname : all_field_names) {
append_to_map<f32>(fname, data, dic_out);
append_to_map<f32_2>(fname, data, dic_out);
append_to_map<f32_3>(fname, data, dic_out);
append_to_map<f32_4>(fname, data, dic_out);
append_to_map<f32_8>(fname, data, dic_out);
append_to_map<f32_16>(fname, data, dic_out);
append_to_map<f64>(fname, data, dic_out);
append_to_map<f64_2>(fname, data, dic_out);
append_to_map<f64_3>(fname, data, dic_out);
append_to_map<f64_4>(fname, data, dic_out);
append_to_map<f64_8>(fname, data, dic_out);
append_to_map<f64_16>(fname, data, dic_out);
append_to_map<u32>(fname, data, dic_out);
append_to_map<u64>(fname, data, dic_out);
append_to_map<u32_3>(fname, data, dic_out);
append_to_map<u64_3>(fname, data, dic_out);
append_to_map<i64_3>(fname, data, dic_out);
}
tdavidcl
requested changes
Jan 2, 2026
Member
tdavidcl
left a comment
tdavidcl
left a comment
There was a problem hiding this comment.
I will try to review in greater datil ASAP. But in the meantime i see two aspects that should be addressed.
1 - Use std::vector instead of std::deque for direct lookup
2 - Wrap the Layout list in a class, something like:
class PatchDataLayout {
<...>
std::vector<std::shared_ptr<PatchDataLayerLayout>> layer_layouts;
std::shared_ptr<PatchDataLayerLayout> & get_layer_layout(size_t id){
return shambase::get_check_ref(layer_layouts.at(id));
}
/// const overload of get_layer_layaout_ptr
const std::shared_ptr<PatchDataLayerLayout> & get_layer_layout_ptr(size_t id) const {
return layer_layouts.at(id);
}
PatchDataLayerLayout & get_layer_layout_ptr(size_t id){
return layer_layouts.at(id);
}
<...>
}Which would make the logic of layers much cleaner, especially since we will have to forward a list of layers to the constructor of PatchData (which will also be a list of PatchDataLayer(s) )
tdavidcl
requested changes
Jan 7, 2026
Member
tdavidcl
left a comment
tdavidcl
left a comment
There was a problem hiding this comment.
Some comments related to formatting.
Please also fix pre-commit @ authorship so that we can test compilation
Comment on lines
+45
to
+49
| inline void create_layers(size_t nlayers) { | ||
| for (size_t idx = 0; idx < nlayers; idx++) { | ||
| layer_layouts.push_back(std::make_shared<shamrock::patch::PatchDataLayerLayout>()); | ||
| } | ||
| } |
Member
There was a problem hiding this comment.
Suggested change
| inline void create_layers(size_t nlayers) { | |
| for (size_t idx = 0; idx < nlayers; idx++) { | |
| layer_layouts.push_back(std::make_shared<shamrock::patch::PatchDataLayerLayout>()); | |
| } | |
| } |
Do it in the constructor
Member
|
In general the idea is that this class would have a constructor to select the number of layer PatchDataLayout(u32 nlayers = 1) then we can just swap the PatchDataLayerLayout by PatchDataLayout and replace all calls to PatchDataLayerLayout through get_layer_ref which make the changes minimal considering that it touches core functionalities |
Contributor
Workflow reportworkflow report corresponding to commit 894b509 Light CI is enabled. This will only run the basic tests and not the full tests. Pre-commit check reportPre-commit check: ✅ Test pipeline can run. Doxygen diff with
|
tdavidcl
reviewed
Jan 8, 2026
| using SchedulerPatchData = shamrock::scheduler::SchedulerPatchData; | ||
|
|
||
| std::shared_ptr<shamrock::patch::PatchDataLayerLayout> pdl_ptr; | ||
| shamrock::patch::PatchDataLayout pdl_ptr; |
Member
There was a problem hiding this comment.
Suggested change
| shamrock::patch::PatchDataLayout pdl_ptr; | |
| std::shared_ptr<shamrock::patch::PatchDataLayout> pdl_ptr; |
Member
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the PatchDataLayerLayout to be managed within a new PatchDataLayout class, which acts as a container for multiple PatchDataLayerLayout instances. This change allows for better handling of multiple data layers. The ShamrockCtx and PatchScheduler classes have been updated to reflect this new structure, including changes to constructors and data access methods. Test files have also been adjusted to use the new PatchDataLayout.
However, there are a few areas that could be improved for maintainability, efficiency, and clarity, particularly regarding the handling of multiple layers in PatchScheduler and the naming of certain methods.
| std::vector<std::shared_ptr<shamrock::patch::PatchDataLayerLayout>> layer_layouts; | ||
| size_t get_layer_count() const { return layer_layouts.size(); } | ||
|
|
||
| inline std::shared_ptr<PatchDataLayerLayout> &get_layer_layout_ref(size_t idx = 0) { |
There was a problem hiding this comment.
Returning a non-const reference to a std::shared_ptr can be risky. If the returned shared_ptr itself is modified (e.g., reset or assigned a new pointer), any existing references to it would become dangling. Consider returning const std::shared_ptr<PatchDataLayerLayout>& if the intent is only to read the shared pointer, or std::shared_ptr<PatchDataLayerLayout> by value if the caller needs to take ownership or manage its lifetime independently.
| return layer_layouts.at(idx); | ||
| } | ||
|
|
||
| inline PatchDataLayerLayout &get_layer_layout_ptr(size_t idx = 0) { |
There was a problem hiding this comment.
The method name get_layer_layout_ptr is misleading as it returns a reference (PatchDataLayerLayout &), not a pointer. This could lead to confusion for users expecting a raw pointer or std::shared_ptr.
Suggested change
| inline PatchDataLayerLayout &get_layer_layout_ptr(size_t idx = 0) { | |
| inline PatchDataLayerLayout &get_layer_layout_ref_to_val(size_t idx = 0) { |
Comment on lines
+46
to
+50
| inline void create_layers(size_t nlayers) { | ||
| for (size_t idx = 0; idx < nlayers; idx++) { | ||
| layer_layouts.push_back(std::make_shared<shamrock::patch::PatchDataLayerLayout>()); | ||
| } | ||
| } |
There was a problem hiding this comment.
The create_layers method unconditionally adds new layers. If this method is called multiple times, it will keep adding layers, which might not be the intended behavior. Consider adding a check to clear existing layers or throw an exception if layer_layouts is not empty, depending on the desired semantics for re-initialization.
| const std::shared_ptr<shamrock::patch::PatchDataLayerLayout> &pdl_ptr, | ||
| u64 crit_split, | ||
| u64 crit_merge); | ||
| PatchScheduler(const shamrock::patch::PatchDataLayout pdl_ptr, u64 crit_split, u64 crit_merge); |
There was a problem hiding this comment.
Passing pdl_ptr by value (const shamrock::patch::PatchDataLayout pdl_ptr) will create a copy of the PatchDataLayout object, including its std::vector of std::shared_ptrs. While shared_ptrs handle copying efficiently, if PatchDataLayout were to become large or contain other non-trivial members, this could incur unnecessary overhead. Consider passing it by const& to avoid copying.
PatchScheduler(const shamrock::patch::PatchDataLayout &pdl_ptr, u64 crit_split, u64 crit_merge);
Comment on lines
+138
to
+142
| throw std::invalid_argument( | ||
| std::string("the main field is not of the correct type to call this function\n") | ||
| + "fct called : " + __PRETTY_FUNCTION__ | ||
| + shambase::format("current patch data layout of index {} : ", layer_idx) | ||
| + layer_layout->get_description_str()); |
There was a problem hiding this comment.
The error message here is quite verbose and includes __PRETTY_FUNCTION__ and shambase::format. While useful for debugging, for a user-facing std::invalid_argument exception, a more concise and actionable message might be preferable. The current message could be simplified to focus on the core issue: the main field type mismatch for the given layer.
| ctx.pdata_layout_new(); | ||
| *ctx.pdl = jmeta_patch.at("patchdata_layout").get<patch::PatchDataLayerLayout>(); | ||
| ctx.pdl.get_layer_layout_ptr(0) = jmeta_patch.at("patchdata_layout") | ||
| .get<patch::PatchDataLayerLayout>(); // ->astodo ->hc |
There was a problem hiding this comment.
The comment // ->stodo ->hc indicates a hardcoded value or a TODO item. This suggests that the initialization of ctx.pdl.get_layer_layout_ptr(0) might be a temporary solution and needs to be generalized to handle multiple layers dynamically, rather than always assuming layer 0. This is a maintainability concern.
| patch::PatchDataLayer pdat = patch::PatchDataLayer::deserialize_buf( | ||
| ser, | ||
| std::make_shared<patch::PatchDataLayerLayout>( | ||
| ctx.pdl.get_layer_layout_ptr(0))); // ->astodo ->hc |
There was a problem hiding this comment.
Similar to the previous comment, // ->stodo ->hc here suggests that the deserialization is hardcoded to layer 0. This will need to be addressed if the system is to support multiple PatchDataLayerLayouts being loaded from a dump file.
| const std::shared_ptr<shamrock::patch::PatchDataLayerLayout> &pdl_ptr, | ||
| u64 crit_split, | ||
| u64 crit_merge) | ||
| const shamrock::patch::PatchDataLayout pdl_ptr, u64 crit_split, u64 crit_merge) |
There was a problem hiding this comment.
Consistent with the header, pdl_ptr is passed by value. As noted before, consider passing by const& to avoid unnecessary copying of the PatchDataLayout object.
Suggested change
| const shamrock::patch::PatchDataLayout pdl_ptr, u64 crit_split, u64 crit_merge) | |
| const shamrock::patch::PatchDataLayout &pdl_ptr, u64 crit_split, u64 crit_merge) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
std::vector) ofPatchDataLayerLayoutPatchDataLayerLayoutto be the first element of the list