Swap to using 'get_node_ids' that may be sharded or fetch a split, etc#467
Swap to using 'get_node_ids' that may be sharded or fetch a split, etc#467kmontemayor2-sc merged 11 commits intomainfrom
Conversation
|
/unit_test_py |
|
/integration_test |
|
/e2e_test |
GiGL Automation@ 17:56:25UTC : 🔄 @ 19:10:59UTC : ✅ Workflow completed successfully. |
GiGL Automation@ 17:56:36UTC : 🔄 |
GiGL Automation@ 17:56:55UTC : 🔄 @ 19:17:55UTC : ✅ Workflow completed successfully. |
|
/unit_test_py |
|
/integration_test |
|
/e2e_test |
GiGL Automation@ 23:31:16UTC : 🔄 @ 24:39:48UTC : ✅ Workflow completed successfully. |
GiGL Automation@ 23:31:19UTC : 🔄 |
GiGL Automation@ 23:31:21UTC : 🔄 @ 24:55:47UTC : ✅ Workflow completed successfully. |
|
/unit_test_py |
|
/integration_test |
GiGL Automation@ 23:32:48UTC : 🔄 @ 24:51:43UTC : ✅ Workflow completed successfully. |
GiGL Automation@ 23:32:52UTC : 🔄 |
|
/unit_test_py |
|
/integration_test |
|
/e2e_test |
GiGL Automation@ 18:21:41UTC : 🔄 @ 19:40:00UTC : ✅ Workflow completed successfully. |
GiGL Automation@ 18:21:49UTC : 🔄 @ 19:43:14UTC : ✅ Workflow completed successfully. |
GiGL Automation@ 18:21:52UTC : 🔄 |
mkolodner-sc
left a comment
mkolodner-sc
left a comment
There was a problem hiding this comment.
Thanks Kyle! Left a few small comments, generally LGTM.
In the future, it might be easier to review this if the testing utility changes were moved to a separate PR from the get_node_ids change here.
svij-sc
left a comment
svij-sc
left a comment
There was a problem hiding this comment.
Approved from an API change perspective.
Provided some suggestions for further improvement.
Will leave it to @mkolodner-sc to review the details / implementation.
mkolodner-sc
left a comment
mkolodner-sc
left a comment
There was a problem hiding this comment.
This LGTM, thanks!
|
🆒 |
kmontemayor2-sc
requested review from
nshah-sc,
xgao4-sc,
yliu2-sc and
zfan3-sc
as code owners
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
4 participants
Scope of work done
Per discussion in #438, migrating to some
get_node_idswhich is more flexible and can fetch a split and optionally shard, etc.Additionally, breaking out the dataset building utils to their own (tested) file,
tests/test_assets/distributed/test_dataset.pyand then creatingremote_dist_dataset_test.pyas unit tests for RDI.Where is the documentation for this feature?: N/A
Did you add automated tests or write a test plan?
Updated Changelog.md? NO
Ready for code review?: NO