[Validate] Track-level metrics upload #375
Conversation
|
|
||
| self.tqdm_bar = tqdm_notebook.tqdm | ||
| self._connection = Connection(self.api_key, self.endpoint) | ||
| self.connection = Connection(self.api_key, self.endpoint) |
There was a problem hiding this comment.
Tricky design decision here. Track needs access to at least a Connection object to be able to call the API to upload track metadata, preferably NucleusClient access for consistency with other classes like Model. However, a Validate instance is created from NucleusClient, and so attempting to import Track from ScenarioTest, from where it is created in get_items, will cause a circular import. The lessermost evil was to update Track.from_json to take a Connection instead of a NucleusClient.
The tradeoff here was exposing connection as a public property of NucleusClient so to access this property when creating Tracks in cases when only a NucleusClient is available. This should be safe, since a Connection consists of two public properties––api_key and endpoint.
Lmk if there are any issues with this approach.
There was a problem hiding this comment.
Yeah this is fine.
I think passing around a connection is a newer approach than passing the client. I actually don't see any reason for a class having to have full access to the client. We should probably refactor everything to pass around the connection object itself.
I don't see any harm in doing things this way so ... let's go ahead 🙂
| ) | ||
| if level == EntityLevel.TRACK: | ||
| return [ | ||
| Track.from_json(track, connection=self.connection) |
There was a problem hiding this comment.
This line is the reason for the long justification above 😤
There was a problem hiding this comment.
Haha, the struggle is real 🥇
| test_slice, | ||
| slice_items, | ||
| ): | ||
| # Arrange |
There was a problem hiding this comment.
Refactored the test_scenario_test_get_* methods via AAA-C bc of my OCD
There was a problem hiding this comment.
Feel free to drop the link next to one of the comments 👍
There was a problem hiding this comment.
I think it's ok, pretty common paradigm and future tests can use the structure of these functions as inspiration. Don't feel too fond about linking to Medium lmao
|
|
||
| self.tqdm_bar = tqdm_notebook.tqdm | ||
| self._connection = Connection(self.api_key, self.endpoint) | ||
| self.connection = Connection(self.api_key, self.endpoint) |
There was a problem hiding this comment.
Yeah this is fine.
I think passing around a connection is a newer approach than passing the client. I actually don't see any reason for a class having to have full access to the client. We should probably refactor everything to pass around the connection object itself.
I don't see any harm in doing things this way so ... let's go ahead 🙂
| ) | ||
| if level == EntityLevel.TRACK: | ||
| return [ | ||
| Track.from_json(track, connection=self.connection) |
There was a problem hiding this comment.
Haha, the struggle is real 🥇
| test_slice, | ||
| slice_items, | ||
| ): | ||
| # Arrange |
There was a problem hiding this comment.
Feel free to drop the link next to one of the comments 👍
Added support for uploading metrics at the track level from the SDK.
Tested everything in https://github.com/scaleapi/scaleapi/pull/50768. Will merge after the
scaleapichanges are deployed and tests pass.