data: define interface for read_last_scalars
#6657
Conversation
| ctx=None, | ||
| *, | ||
| experiment_id, | ||
| plugin_name, |
There was a problem hiding this comment.
Since we're requiring plugin_name to be specified, I would think having the method be read_latest_values, or something like this, would be more generic/flexible, and we can say in the doc string that it's possible that not all plugins are supported by all implementations.
Although the return value does get a bit more difficult to specify and deal with, so if you prefer this because of this reason, I'm ok with it.
There was a problem hiding this comment.
I specified plugin_name because read_scalars also requires plugin_name. I prefer the simpler definition for just the scalars, in this case do you think we can just remove the plugin_name arg?
There was a problem hiding this comment.
I decided to remove the plugin_name completely. I think it's unlikely that we'll have a new plugin type that contains scalar data in the near future. And since scalar seems to be the most used type, we don't have to generalize it to read_last_values just yet, wdyt?
There was a problem hiding this comment.
Oh, sorry for the late response. I thought I had replied to this. Technically, the plugin type is somewhat independent from the data type, e.g. there's a custom_scalars plugin in our examples, I believe, which also uses scalar values.
For this reason, and to be consistent with the rest of the interface for the data provider, I'd lean a bit towards keeping the plugin_name... but this is ok too. I agree it's unlikely we'll ever need anything other than "scalars", and if we do, we can change it later (but again, unlikely).
There was a problem hiding this comment.
Great point, added it back.
There was a problem hiding this comment.
Also I didn't realize I also needed to implement this method in event_processing/data_provider. PTAL, thanks!
yatbear
force-pushed
the
read_last_scalars
branch
from
34c7f69 to
f30a9f8
Compare
| ctx=None, | ||
| *, | ||
| experiment_id, | ||
| plugin_name, |
There was a problem hiding this comment.
Oh, sorry for the late response. I thought I had replied to this. Technically, the plugin type is somewhat independent from the data type, e.g. there's a custom_scalars plugin in our examples, I believe, which also uses scalar values.
For this reason, and to be consistent with the rest of the interface for the data provider, I'd lean a bit towards keeping the plugin_name... but this is ok too. I agree it's unlikely we'll ever need anything other than "scalars", and if we do, we can change it later (but again, unlikely).
…ave scalar data (e.g. custom scalars plugin)
This will be used for
hparams.backend_context.read_last_scalars.Googlers, see b/292102513 for more context.
Tested internally at cl/577906864.
#hparams