Show Experiment Remote Status #4324
Conversation
mattseddon
force-pushed
the
show-exp-remote-status
branch
from
e403be6 to
c928747
Compare
| @@ -68,6 +69,18 @@ export class GitReader extends GitCli { | |||
| } | |||
| } | |||
|
|
|||
| public async getRemoteExperimentRefs(cwd: string): Promise<string> { | |||
| const options = getOptions({ | |||
There was a problem hiding this comment.
[F] Bit concerned with the performance of this. Have spent considerable time trying to find a way around but haven't come up with anything yet.
There was a problem hiding this comment.
I think we should start doing status non-essential heavy checks in background for experiments. If we already do - what is your concern then?
There was a problem hiding this comment.
do you mean polling?
There was a problem hiding this comment.
We should look at setting up WebSockets in Studio.
There was a problem hiding this comment.
I mean that it's important that all these statutes don't affect UI. It's fine that some icons/ statuses are missing and appear in a while. WebSockets won't be enough since we need to run it the first time? (I might be missing the context of what we do here btw).
| return Toast.delayProgressClosing(15000) | ||
| } catch (error: unknown) { | ||
| void updateOnCompletion() | ||
| throw error |
There was a problem hiding this comment.
[F] This will be caught by InternalCommands and offer to show the output channel to the user (which will show the underlying error). We just need to update the data appropriately before that error is thrown.
| } | ||
|
|
||
| public unsetPushing(ids: string[]) { | ||
| this.experiments.unsetPushing(ids) |
There was a problem hiding this comment.
[F] We don't notify here because we want to fetch the data again.
There was a problem hiding this comment.
I've moved update into here to make sure the function is never accidentally called without it.
| } | ||
|
|
||
| .plotBox { | ||
| @extend %iconBox; |
There was a problem hiding this comment.
[Q] Should I be using cx instead of extending a base class?
There was a problem hiding this comment.
Extending a base class sounds like a better option to me since this class isn't conditionally added.
Though %iconBox should be moved to the "Extendable Silent Rules" section (starts on line 15).
| export const ExperimentStatusIndicator: React.FC< | ||
| ExperimentStatusIndicatorProps | ||
| > = ({ id, status, gitRemoteStatus }) => { | ||
| if (isRunning(status) || gitRemoteStatus === GitRemoteStatus.PUSHING) { |
There was a problem hiding this comment.
[I] This status field should be renamed to executorStatus or similar in a follow up
|
|
||
| if (gitRemoteStatus === GitRemoteStatus.ON_REMOTE) { | ||
| return ( | ||
| <CellHintTooltip tooltipContent="Experiment on remote"> |
There was a problem hiding this comment.
Should we make the cloud's icon the same width as other icons (aka 16px instead of 20px)? This will also keep all experiments along the same alignment:
There was a problem hiding this comment.
for some reason I thought 16 was the default, updated
| } | ||
|
|
||
| .plotBox { | ||
| @extend %iconBox; |
There was a problem hiding this comment.
Extending a base class sounds like a better option to me since this class isn't conditionally added.
Though %iconBox should be moved to the "Extendable Silent Rules" section (starts on line 15).
| @@ -68,6 +69,18 @@ export class GitReader extends GitCli { | |||
| } | |||
| } | |||
|
|
|||
| public async getRemoteExperimentRefs(cwd: string): Promise<string> { | |||
| const options = getOptions({ | |||
| args: [Command.LS_REMOTE, DEFAULT_REMOTE, 'refs/exps/*'], | |||
There was a problem hiding this comment.
There are no "remote watchers" to trigger table updates for things like other team members deleting your experiments from the remote or even a user running dvc exp remove -g origin themselves.
Might have already been discussed, but have we considered adding a refresh button that will run this command again? Could be a generic one that refreshes all table data entirely or adding an extra action to our "remote state" tooltips 🤔
There was a problem hiding this comment.
We have purposefully avoided a refresh button up to this point. We may have to introduce one for remote action but let's hold off until after we decide on a strategy for fetching the data.
|
Code Climate has analyzed commit ad0f089 and detected 0 issues on this pull request. The test coverage on the diff in this pull request is 96.3% (85% is the threshold). This pull request will bring the total coverage in the repository to 95.2%. View more on Code Climate. |
Related to #3574
This PR extends the experiment status indicator to the right of an experiment in the table.
The indicator will now show whether or not an experiment has been pushed to the git remote (the extension uses
git ls-remote origin 'refs/exps/*'to determine whether or not an experiment is on the remote).If the experiment is not on the remote then an icon is shown that the user can click to push. If it is on the remote then an icon is shown to indicate that status. If the experiment is pushed via the extension then we show a spinner to indicate that something is happening.
Limitations:
dvc exp remove -g origin <exp-name>themselves.Demo
Screen.Recording.2023-07-24.at.7.31.46.pm.mov
Screen.Recording.2023-07-24.at.7.35.28.pm.mov
cc @dberenbaum