Skip to content
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

(bugfix) Make Experiments Overview Page Show Dates & Runs #1369

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

vinayan3
Copy link

@vinayan3 vinayan3 commented Aug 2, 2024

Date fix

The server is sending data in camel case which causes the experiment overview page to show invalid date instead of the actual date. The expected data type is here: https://github.com/G-Research/fasttrackml-ui-aim/blob/release/v3.17.5/src/src/modules/core/api/experimentsApi/types.ts#L84-L90

Before the change
Screenshot from 2024-08-01 17-58-25

After the change
Screenshot from 2024-08-01 17-58-51

Runs fix

When you click the runs tab it make an API call to the AIM end points for GET /runs/search/run. The UI's code sets the experiment as a query see:
https://github.com/G-Research/fasttrackml-ui-aim/blob/release/v3.17.5/src/src/pages/Experiment/components/ExperimentRunsTab/ExperimentRunsTable/useExperimentRunsTable.tsx#L224-L228

If ExperimentNamesis empty it shouldn't be included in the query on the DB.

jescalada
jescalada previously approved these changes Aug 7, 2024
Copy link
Collaborator

@jescalada jescalada left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested all the issues we were having in #1334 and it looks good to me! The code looks fine as well.

Pinging @suprjinx for feedback on the query just in case.

@jescalada
Copy link
Collaborator

@vinayan3 It seems however that some of the Go tests related to Run Searching are failing ATM. Let's make sure that those are passing first 😃

@jescalada jescalada linked an issue Aug 7, 2024 that may be closed by this pull request
@vinayan3
Copy link
Author

vinayan3 commented Aug 7, 2024

@jescalada so I'll need to change the test NoExperimentNamesNoResults . The behavior is now different where if you don't provide an experiment name then it allows all. Instead it'll be NoExpeirmentNamesResultsFromQuery.

Copy link
Contributor

@suprjinx suprjinx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great idea -- but I think we need to limit runs returned when no experiment names are provided. Additionally, I think SearchRuns should always require the experiment selection -- maybe a new repository function for use in the overview which returns most recent runs?

@vinayan3
Copy link
Author

Great idea -- but I think we need to limit runs returned when no experiment names are provided. Additionally, I think SearchRuns should always require the experiment selection -- maybe a new repository function for use in the overview which returns most recent runs?

Could a solution here to be add a limit field to the API request and make experiment names or limit required?

Otherwise, I'm fine with adding a new request for the overview call.

@suprjinx
Copy link
Contributor

@vinayan3 -- I think maybe we should keep your changes for creation_time, end_time serialization in this PR, and then open a new PR in the UI repo. The runs tab in the experiment overview is issuing a request like this:
http://localhost:5000/aim/api/runs/search/run?limit=50&exclude_params=true&q=run.experiment%20==%20%27my-experiment%27

But we need it to issue one like this:
http://localhost:5000/aim/api/runs/search/run?q=&experiment_names=my-experiment&limit=45

So we would just alter the fetchExperimentRuns function you highlighted to structure the params differently, and it should work.

@vinayan3 vinayan3 force-pushed the fix_experiments_overview_page_work branch from 9d7b17b to 02d5db1 Compare August 21, 2024 02:52
@vinayan3
Copy link
Author

@vinayan3 -- I think maybe we should keep your changes for creation_time, end_time serialization in this PR, and then open a new PR in the UI repo. The runs tab in the experiment overview is issuing a request like this: http://localhost:5000/aim/api/runs/search/run?limit=50&exclude_params=true&q=run.experiment%20==%20%27my-experiment%27

But we need it to issue one like this: http://localhost:5000/aim/api/runs/search/run?q=&experiment_names=my-experiment&limit=45

So we would just alter the fetchExperimentRuns function you highlighted to structure the params differently, and it should work.

I'll have to work through hitting a new endpoint in the UI later this week.

@suprjinx
Copy link
Contributor

@vinayan3 -- I think maybe we should keep your changes for creation_time, end_time serialization in this PR, and then open a new PR in the UI repo. The runs tab in the experiment overview is issuing a request like this: http://localhost:5000/aim/api/runs/search/run?limit=50&exclude_params=true&q=run.experiment%20==%20%27my-experiment%27
But we need it to issue one like this: http://localhost:5000/aim/api/runs/search/run?q=&experiment_names=my-experiment&limit=45
So we would just alter the fetchExperimentRuns function you highlighted to structure the params differently, and it should work.

I'll have to work through hitting a new endpoint in the UI later this week.

Cool thank you! I think it's not even a new endpoint, just changing the param structure in the ajax request (from q to experiment_names).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: Run selector on Run page doesn't show relevant info
3 participants