-
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
Initial MLI schemas and MessageHandler class #607
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## mli-feature #607 +/- ##
================================================
- Coverage 71.53% 53.35% -18.18%
================================================
Files 78 79 +1
Lines 6000 6187 +187
================================================
- Hits 4292 3301 -991
- Misses 1708 2886 +1178
|
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.
Just some quick questions/comments/suggestions
smartsim/_core/mli/mli_schemas/request/request_attributes/request_attributes.capnp
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.
Looks good! Some general comments, possibly tending to over-engineering.
@staticmethod | ||
def _assign_custom_request_attributes( | ||
request: request_capnp.Request, | ||
custom_attrs: t.Union[ |
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'm slightly concerned by extensibility/maintainability of this type: do we need to keep coming to this portion of the code and add new XyzRequestAttributes
every time we support a new library? Is there a more flexible way? ("No" is an acceptable answer).
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 am also concerned about the extensibility/maintainability of this type! Currently I think we will just need to come back and add to it because I need to check the passed in schema name and set the union field depending on that. So it might be no for now, but hopefully I can come up with a better way in the future.
This PR adds the Capnproto schemas and initial MessageHandler class and tests. This is very much a work in progress!