Skip to content

Update resource config wrapper / validation check to support graph store mode#383

Merged
kmontemayor2-sc merged 5 commits intomainfrom
kmonte/update-rc-wrapper-for-graph-store
Nov 18, 2025
Merged

Update resource config wrapper / validation check to support graph store mode#383
kmontemayor2-sc merged 5 commits intomainfrom
kmonte/update-rc-wrapper-for-graph-store

Conversation

@kmontemayor2-sc
Copy link
Copy Markdown
Collaborator

Scope of work done

As mentioned, support VertexAIGraphStore config message in our wrapper and validation check now.

Also slop-up some tests ;)

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

Comment thread python/gigl/src/validation_check/libs/resource_config_checks.py Outdated
Comment thread python/gigl/src/validation_check/libs/resource_config_checks.py Outdated
Comment thread python/tests/unit/src/common/utils/types/pb_wrappers/gigl_resource_config_test.py Outdated
Copy link
Copy Markdown
Collaborator

@svij-sc svij-sc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some comments.

Generally I am conflicted to see the value of adding all these tests. A lot of these tests are essentially testing asserts that an object is correctly housing another object it was initialized with.

If anything, there may be more value in testing the config validator raising issues when configs are not correctly formulated - which is higher order and will implicitly capture most tests here.

Comment thread python/tests/unit/src/common/utils/types/pb_wrappers/gigl_resource_config_test.py Outdated
Comment thread python/tests/unit/src/common/utils/types/pb_wrappers/gigl_resource_config_test.py Outdated
Comment thread python/tests/unit/src/common/utils/types/pb_wrappers/gigl_resource_config_test.py Outdated
Comment thread python/tests/unit/src/common/utils/types/pb_wrappers/gigl_resource_config_test.py Outdated
Comment thread python/tests/unit/src/common/utils/types/pb_wrappers/gigl_resource_config_test.py Outdated
Copy link
Copy Markdown
Collaborator

@svij-sc svij-sc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer that we dont add new* tests to test supposedly deprecated code.

Also, unsure if you had a chance to look at #383 (review) yet.
Nonetheless, approving since overall quality is still improving here for our testing suite.
Thanks for the work!

@kmontemayor2-sc
Copy link
Copy Markdown
Collaborator Author

Generally I am conflicted to see the value of adding all these tests. A lot of these tests are essentially testing asserts that an object is correctly housing another object it was initialized with.

I think that there is actually non-trivial logic in the wrapper, see trainer_config, get_resource_labels, and shared_resource_config, and that is worth testing.

As for testing the pass through stuff, agreed it's less useful but I don't think it hurts - it may catch a reggression in the future and ime these sorts of unit tests are not that costly to maintain. And if they are costly, we can always delete them :P

If anything, there may be more value in testing the config validator raising issues when configs are not correctly formulated - which is higher order and will implicitly capture most tests here.

We do this in this PR, too!

@svij-sc
Copy link
Copy Markdown
Collaborator

svij-sc commented Nov 14, 2025

I think that there is actually non-trivial logic in the wrapper, see trainer_config, get_resource_labels, and shared_resource_config, and that is worth testing.

Totally, this part re: labels, et al is probably worth it. But others are just asserting what I mentioned prior

Copy link
Copy Markdown
Collaborator

@mkolodner-sc mkolodner-sc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Kyle! Did a pass

Comment thread python/gigl/src/common/types/pb_wrappers/gigl_resource_config.py
Comment thread python/gigl/src/validation_check/libs/resource_config_checks.py
Comment thread python/gigl/src/validation_check/libs/resource_config_checks.py
Comment thread python/gigl/src/validation_check/libs/resource_config_checks.py Outdated
Comment thread python/tests/unit/src/common/utils/types/pb_wrappers/gigl_resource_config_test.py Outdated
Comment thread python/tests/unit/src/validation/lib/resource_config_checks_test.py Outdated
Copy link
Copy Markdown
Collaborator

@mkolodner-sc mkolodner-sc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Kyle!

@kmontemayor2-sc kmontemayor2-sc added this pull request to the merge queue Nov 18, 2025
@github-merge-queue github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Nov 18, 2025
@kmontemayor2-sc kmontemayor2-sc added this pull request to the merge queue Nov 18, 2025
Merged via the queue into main with commit deb97e0 Nov 18, 2025
5 checks passed
@kmontemayor2-sc kmontemayor2-sc deleted the kmonte/update-rc-wrapper-for-graph-store branch November 18, 2025 21:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants