-
Notifications
You must be signed in to change notification settings - Fork 37
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
Add integration of dragon-based event broadcasting #710
Conversation
Revert tensor/model key, update tests, enhance logging Tweak exception naming to follow standard Test remote queue delay and nowait Remove large test timeout Update new tests w/fsk reversion Modify import order for remote tester
…agon utils module, fix missing test env vars, ...
… reuse some test fixtures for speed
…measurement error
… use fixtures for pytest ddict creation
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.
Really great additions that get us closer to the online notifications. Mostly minor questions/comments
smartsim/_core/mli/infrastructure/storage/backbone_feature_store.py
Outdated
Show resolved
Hide resolved
smartsim/_core/mli/infrastructure/storage/backbone_feature_store.py
Outdated
Show resolved
Hide resolved
smartsim/_core/mli/infrastructure/storage/backbone_feature_store.py
Outdated
Show resolved
Hide resolved
smartsim/_core/mli/infrastructure/storage/backbone_feature_store.py
Outdated
Show resolved
Hide resolved
smartsim/_core/mli/infrastructure/storage/backbone_feature_store.py
Outdated
Show resolved
Hide resolved
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.
A few comments, but overall looks impressive
smartsim/_core/mli/infrastructure/control/request_dispatcher.py
Outdated
Show resolved
Hide resolved
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.
The other reviewers had already caught any of the questions or suggestions I had. This is really well done!
… consumers, moar tests
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 must commend thee on thy splendid work! I have conjured a few suggestions that might enhance your creation, but feel free to disregard any that do not align with your vision. I have perused about halfway through and shall return to complete my review with haste.
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.
Some more comments, slowly looking through
smartsim/_core/mli/infrastructure/storage/backbone_feature_store.py
Outdated
Show resolved
Hide resolved
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.
Partial review, stopping at consumer.py
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.
Only a few nitpicks otherwise looks great!
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.
Reviewed rest of files excluding /tests, incredible work!
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.
LGTM!
This PR integrates event publishers and consumers in
ProtoClient
andDragonBackend