timeseries: match selection using same logic #5334
Conversation
| regex.test(item.experimentName) || | ||
| regex.test(legacyRunName) | ||
| return matchRunToRegex( | ||
| {...item.run, ...item}, |
There was a problem hiding this comment.
This illustrates both the curse and the strength of typescript. This is my understanding of what's happening here:
(1) You have a function that really just wants {runName, experimentName, experimentAlias}.
(2) But your function accepts a type that is actually {id, name, startTime, experimentName, experimentAlias}. "name" is implicitly the run name, but it's not too implicit because at least you named the type RunWithExperimentName.
(3) But your call site is passing type {id, name, startTime, run, experimentAlias, experimentName, selected, hparams, metrics} . I have to reason out what from item.run and item is actually necessary for the call to matchRunToRegex() and what is unnecessary.
You could fix the disparity between (1) and (2) by instead using an interface or type that is just {runName, experimentName, experimentAlias}. But then you can't really address the disparity with (3)... caller could still pass {runName: item.run, ...item} and I'd still have to see what the function really wants.
So it's kind of a mess and kind of powerful and I'm not sure if I like it or dislike it. You can do what you want with this comment.
There was a problem hiding this comment.
It may be logically messy but type wise, I think it is okay for users of the API to provide superset as long as it is strongly type enforced by the compiler.
Still, I've made some changes to explicitly require only very tight subset of the Run that I require in the method.
| experimentAlias: string; | ||
| } | ||
|
|
||
| export function matchRunToRegex( |
There was a problem hiding this comment.
Some documentation would be nice:
Returns true if run name matches given regex. If shouldMatchExperiment is true then will return true if one of run name, experiment name, experiment alias, or the legacy run format of "[experiment alias]/[run name]" matches the given regex.
Also possibly document: The latter case is preferable to use when comparing experiments.
There was a problem hiding this comment.
Alternatively consider that you have actually two different API calls:
matchRunNameToRegex(runName: string, regexString: string).
matchRunAndExperimentNamesToRegex(runAndExperimentName: {runName, experimentName, experimentAlias}, regexString: string).
tensorboard/webapp/util/matcher.ts
Outdated
| * - Invalid regex always return false. | ||
| * - Regex matches are case insensitive. | ||
| * - Regex matches are not anchored. | ||
| * - Matches name of a run. |
There was a problem hiding this comment.
I would clarify that it returns true if the run matches the regex. "matches" on its own is vague.
There was a problem hiding this comment.
Added the clarifying text. However, given the title of the jsdoc states,
Matches an entry based on regex and business logic.
I think it would be self-explanatory that matches are against regex. :(
Previously, to allow regex string also match run selection, we have extend the utility selector to match run selection against the regex. The logic, however, was massively flawed--instead of matching against name of a run, experiment, alias, and legacy run name, the implementation matched against runId which coincidentally contained run name but was supposed to be an opaque id. This caused an obvious match like `/^foo/` not to match anything especially when an oapque id is not created to start with "foo". In the end, this change basically re-implements the run selection based on regex more correct. In the process, we refactored how a run is matched against regex so the behavior is more congruent in our app.
stephanwlee
force-pushed
the
bug_fix
branch
from
959ae07 to
bf6463c
Compare
Previously, to allow regex string also match run selection, we have extend the utility selector to match run selection against the regex. The logic, however, was massively flawed--instead of matching against name of a run, experiment, alias, and legacy run name, the implementation matched against runId which coincidentally contained run name but was supposed to be an opaque id. This caused an obvious match like `/^foo/` not to match anything especially when an opaque id is not created to start with "foo". In the end, this change basically re-implements the run selection based on regex more correct. In the process, we refactored how a run is matched against regex so the behavior is more congruent in our app.
Previously, to allow regex string also match run selection, we have extend the utility selector to match run selection against the regex. The logic, however, was massively flawed--instead of matching against name of a run, experiment, alias, and legacy run name, the implementation matched against runId which coincidentally contained run name but was supposed to be an opaque id. This caused an obvious match like `/^foo/` not to match anything especially when an opaque id is not created to start with "foo". In the end, this change basically re-implements the run selection based on regex more correct. In the process, we refactored how a run is matched against regex so the behavior is more congruent in our app.
Previously, to allow regex string also match run selection, we have
extend the utility selector to match run selection against the regex.
The logic, however, was massively flawed--instead of matching against
name of a run, experiment, alias, and legacy run name, the
implementation matched against runId which coincidentally contained run
name but was supposed to be an opaque id. This caused an obvious match
like
/^foo/not to match anything especially when an opaque id is notcreated to start with "foo".
In the end, this change basically re-implements the run selection based
on regex more correct.
In the process, we refactored how a run is matched against regex so the
behavior is more congruent in our app.