[Custom Storage 3/3] Add validation for custom storage main #463
Conversation
|
/integration_test |
|
/unit_test_py |
GiGL Automation@ 21:28:30UTC : 🔄 @ 22:58:19UTC : ✅ Workflow completed successfully. |
GiGL Automation@ 21:28:32UTC : 🔄 @ 22:32:34UTC : ✅ Workflow completed successfully. |
…rage-main-validation
…rage-main-validation
…rage-main-validation
…rage-main-validation
…rage-main-validation
…rage-main-validation
mkolodner-sc
left a comment
mkolodner-sc
left a comment
There was a problem hiding this comment.
Thanks Kyle! Left a few small comments
gigl/src/validation_check/libs/gbml_and_resource_config_compatibility_checks.py
Outdated
Show resolved
Hide resolved
gigl/src/validation_check/libs/gbml_and_resource_config_compatibility_checks.py
Outdated
Show resolved
Hide resolved
| def _create_gbml_config_with_both_graph_stores( | ||
| storage_command: str = "python -m gigl.distributed.graph_store.storage_main", | ||
| ) -> GbmlConfigPbWrapper: | ||
| """Create a GbmlConfig with graph_store_storage_config for both trainer and inferencer.""" | ||
| gbml_config = gbml_config_pb2.GbmlConfig() | ||
| gbml_config.trainer_config.graph_store_storage_config.command = storage_command | ||
| gbml_config.inferencer_config.graph_store_storage_config.command = storage_command | ||
| return GbmlConfigPbWrapper(gbml_config_pb=gbml_config) | ||
|
|
||
|
|
||
| def _create_gbml_config_without_graph_stores() -> GbmlConfigPbWrapper: | ||
| """Create a GbmlConfig without graph_store_storage_config for both trainer and inferencer.""" | ||
| gbml_config = gbml_config_pb2.GbmlConfig() | ||
| gbml_config.trainer_config.trainer_args["some_arg"] = "some_value" | ||
| gbml_config.inferencer_config.inferencer_args["some_arg"] = "some_value" | ||
| return GbmlConfigPbWrapper(gbml_config_pb=gbml_config) |
There was a problem hiding this comment.
Would these two functions _create_gbml_config_with_both_graph_stores and _create_gbml_config_without_graph_stores be sufficient for our testing, meaning we wouldn't need the four functions above?
There was a problem hiding this comment.
Sure, cleaned up.
tests/unit/src/validation/lib/gbml_and_resource_config_compatibility_checks_test.py
Outdated
Show resolved
Hide resolved
| ) | ||
|
|
||
|
|
||
| class TestMixedGraphStoreConfigurations(unittest.TestCase): |
There was a problem hiding this comment.
Do we need a test for this? There is no logic in gbml_and_resource_config_check IIUC which couples the trainer and inference checks together, so the above test cases should be sufficient
There was a problem hiding this comment.
Good point, removed.
| # Helper functions for creating GbmlConfig configurations | ||
|
|
||
|
|
||
| def _create_gbml_config_with_trainer_graph_store( |
There was a problem hiding this comment.
ditto here, if we can simplify our tests by just creating one gbml config with both trainer and inferencer graph store and one gbml config without trainer/inferencer graph store that'd be my preference
There was a problem hiding this comment.
Hmm, done.
FWIW I think that creating big blobs with many values set for tests may not be the "cleanest" as it leads to inter-reliance in the future. But in this case we should be fine.
|
/unit_test_py |
GiGL Automation@ 18:31:56UTC : 🔄 @ 19:47:08UTC : ✅ Workflow completed successfully. |
mkolodner-sc
left a comment
mkolodner-sc
left a comment
There was a problem hiding this comment.
This LGTM, thanks Kyle!
Scope of work done
Adding validation checks for custom storage main.
Essentially make sure that if either the resource or task configs are setup for graph store, that both are.
Need to submit this last as otherwise the e2e tests break :P.
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