Add selected dataset ID to URL for sharing by dbczumar · Pull Request #19142 · mlflow/mlflow

dbczumar · 2025-12-01T20:58:30Z

🛠 DevTools 🛠

Install mlflow from this PR

# mlflow
pip install git+https://github.com/mlflow/mlflow.git@refs/pull/19142/merge
# mlflow-skinny
pip install git+https://github.com/mlflow/mlflow.git@refs/pull/19142/merge#subdirectory=libs/skinny

For Databricks, use the following command:

%sh curl -LsSf https://raw.githubusercontent.com/mlflow/mlflow/HEAD/dev/install-skinny.sh | sh -s pull/19142/merge

Related Issues/PRs

#xxx

What changes are proposed in this pull request?

Add selected dataset ID to URL for sharing

How is this PR tested?

Existing unit/integration tests
New unit/integration tests
Manual tests

Does this PR require documentation update?

Release Notes

Is this a user-facing change?

No. You can skip the rest of this section.
Yes. Give a description of this change to be included in the release notes for MLflow users.

What component(s), interfaces, languages, and integrations does this PR affect?

Components

How should the PR be classified in the release notes? Choose one:

rn/none - No description will be included. The PR will be mentioned only by the PR number in the "Small Bugfixes and Documentation Updates" section
rn/breaking-change - The PR will be mentioned in the "Breaking Changes" section
rn/feature - A new user-facing feature worth mentioning in the release notes
rn/bug-fix - A user-facing bug fix worth mentioning in the release notes
rn/documentation - A user-facing documentation change worth mentioning in the release notes

Should this PR be included in the next patch release?

Yes should be selected for bug fixes, documentation updates, and other small changes. No should be selected for new features and larger changes. If you're unsure about the release classification of this PR, leave this unchecked to let the maintainers decide.

What is a minor/patch release?

Minor release: a release that increments the second part of the version number (e.g., 1.2.0 -> 1.3.0).
Bug fixes, doc updates and new features usually go into minor releases.
Patch release: a release that increments the third part of the version number (e.g., 1.2.0 -> 1.2.1).
Bug fixes and doc updates usually go into patch releases.

Yes (this PR will be cherry-picked and included in the next patch release)
No (this PR will be included in the next minor release)

Signed-off-by: dbczumar <corey.zumar@databricks.com>

github-actions · 2025-12-01T21:06:01Z

Documentation preview for fd94a22 is available at:

https://pr-19142--mlflow-docs-preview.netlify.app/docs/latest/

More info

Ignore this comment if this PR does not change the documentation.
The preview is updated when a new commit is pushed to this PR.
This comment was created by this workflow run.
The documentation was built by this workflow run.

danielseong1 · 2025-12-01T23:29:51Z

...xperiment-tracking/pages/experiment-evaluation-datasets/ExperimentEvaluationDatasetsPage.tsx

 import type { EvaluationDataset } from './types';
 import { ExperimentEvaluationDatasetsPageWrapper } from './ExperimentEvaluationDatasetsPageWrapper';
 import { ExperimentEvaluationDatasetsEmptyState } from './components/ExperimentEvaluationDatasetsEmptyState';
+import { useSelectedDatasetBySearchParam } from './hooks/useSelectedDatasetBySearchParam';


is this defined somewhere?

Sorry, forgot to commit it - pushed!

danielseong1 · 2025-12-01T23:39:04Z

...ng/pages/experiment-evaluation-datasets/components/ExperimentEvaluationDatasetsListTable.tsx

+  // Set the selected dataset if we don't already have one,
  // or if the selected dataset went out of scope (e.g. was deleted / not in search)
-  if (!selectedDataset || !datasets.some((d) => d.dataset_id === selectedDataset.dataset_id)) {
+  if (datasets?.length && (!selectedDatasetId || !datasetIds.includes(selectedDatasetId))) {


not technically a problem with this PR, but can we put this whole state-setting block in an useeffect? otherwise this has risk of very easily causing infinite render loops

useEffect(() => { // No datasets -> clear selection if (!datasets?.length) { setSelectedDataset(undefined); setSelectedDatasetId(undefined); return; } // Valid selectedDatasetId from URL -> use it if (selectedDatasetId) { const matchingDataset = datasets.find((d) => d.dataset_id === selectedDatasetId); if (matchingDataset) { setSelectedDataset(matchingDataset); return; } } // No valid selection -> auto-select first const sortedRows = table.getRowModel().rows; if (sortedRows.length > 0) { const firstDataset = sortedRows[0].original; setSelectedDataset(firstDataset); setSelectedDatasetId(firstDataset.dataset_id); } }, [datasets, selectedDatasetId, setSelectedDataset, setSelectedDatasetId, table]);

Totally - done.

otherwise this has risk of very easily causing infinite render loops

I actually observed this while iterating on the PR - glad there's a better way :)

Signed-off-by: dbczumar <corey.zumar@databricks.com>

daniellok-db · 2025-12-02T02:31:56Z

...iment-tracking/pages/experiment-evaluation-datasets/hooks/useSelectedDatasetBySearchParam.ts

+import { useCallback } from 'react';
+import { useSearchParams } from '@mlflow/mlflow/src/common/utils/RoutingUtils';
+
+const QUERY_PARAM_KEY = 'selectedDataset';


nit: add Id to make it clear + align with the name in code?

Suggested change

const QUERY_PARAM_KEY = 'selectedDataset';

const QUERY_PARAM_KEY = 'selectedDatasetId';

daniellok-db · 2025-12-02T02:32:50Z

...xperiment-tracking/pages/experiment-evaluation-datasets/ExperimentEvaluationDatasetsPage.tsx

  const [datasetListHidden, setDatasetListHidden] = useState(false);
  const [selectedDataset, setSelectedDataset] = useState<EvaluationDataset | undefined>(undefined);
  const [isDatasetsLoading, setIsDatasetsLoading] = useState(false);
+  const [selectedDatasetId, setSelectedDatasetId] = useSelectedDatasetBySearchParam();


rather than using the hook here and passing it down to ExperimentEvaluationDatasetsListTable, can we just use it in ExperimentEvaluationDatasetsListTable directly?

oh i think i see what the intention was. can we define a callback that combines setSelectedDataset and setSelectedDatasetId and pass that in instead (similar to what we did in https://github.com/mlflow/mlflow/pull/19142/files#diff-e2232542c52cc97ec1dd208f20ebfc9ded43289282fa3c8f378b8aa1a318445bR109:

Suggested change

const [selectedDatasetId, setSelectedDatasetId] = useSelectedDatasetBySearchParam();

const [selectedDatasetId, setSelectedDatasetId] = useSelectedDatasetBySearchParam();

const onSelectDataset = useCallback((dataset) => {

setSelectedDataset(dataset);

setSelectedDatasetId(dataset.dataset_id);

});

ignore the above comments! once we address https://github.com/mlflow/mlflow/pull/19142/files#r2579393406 it should be taken care of.

daniellok-db · 2025-12-02T02:37:26Z

...ng/pages/experiment-evaluation-datasets/components/ExperimentEvaluationDatasetsListTable.tsx

+  // Sync dataset selection state with URL and available datasets
+  useEffect(() => {


nit: not sure if this needs to be an effect: https://react.dev/learn/you-might-not-need-an-effect

the original was not written with an effect

Yeah, this was guidance from @danielseong1 - I reverted to not use an effect

daniellok-db · 2025-12-02T02:43:02Z

...ng/pages/experiment-evaluation-datasets/components/ExperimentEvaluationDatasetsListTable.tsx

+    // Sync selectedDataset object when selectedDatasetId changes (e.g., from URL)
+    if (selectedDataset?.dataset_id !== selectedDatasetId) {
+      const datasetFromId = datasets?.find((d) => d.dataset_id === selectedDatasetId);
+      if (datasetFromId) {
+        setSelectedDataset(datasetFromId);
+      }
+    }


actually thinking about it further, i think we should just remove selectedDataset as a state field entirely. it should just purely be derived from datasets + selectedDatasetId. it's weird to have two sources of truth here and have to keep them in sync.

can we lift the useSearchEvaluationDatasets call (lines 177 - 185 above) to the parent component, and pass datasets in here? then we can do something like const selectedDataset = datasets.find(dataset => dataset.dataset_id === selectedDatasetId) rather than having it be in state

Awesome! Done :)

- Rename query param key to 'selectedDatasetId' for clarity - Lift useSearchEvaluationDatasets to parent component - Derive selectedDataset from datasets + selectedDatasetId - Remove duplicate state, simplify auto-selection logic Signed-off-by: dbczumar <corey.zumar@databricks.com>

Address review comment to make child component purely presentational by moving the auto-selection useEffect to the parent component. Signed-off-by: dbczumar <corey.zumar@databricks.com>

Signed-off-by: dbczumar <corey.zumar@databricks.com>

daniellok-db

Lg!

dbczumar added 2 commits December 1, 2025 12:27

fix

84ebc20

Signed-off-by: dbczumar <corey.zumar@databricks.com>

fix

ca75017

Signed-off-by: dbczumar <corey.zumar@databricks.com>

github-actions bot added the v3.7.0 label Dec 1, 2025

dbczumar requested review from daniellok-db and danielseong1 December 1, 2025 20:58

github-actions bot added area/evaluation MLflow Evaluation rn/none List under Small Changes in Changelogs. labels Dec 1, 2025

danielseong1 reviewed Dec 1, 2025

View reviewed changes

dbczumar added 2 commits December 1, 2025 15:39

fix

867f138

Signed-off-by: dbczumar <corey.zumar@databricks.com>

fix

4b6815e

Signed-off-by: dbczumar <corey.zumar@databricks.com>

danielseong1 approved these changes Dec 1, 2025

View reviewed changes

daniellok-db reviewed Dec 2, 2025

View reviewed changes

dbczumar added 3 commits December 3, 2025 14:03

Move auto-selection logic to parent component

1eb4f29

Address review comment to make child component purely presentational by moving the auto-selection useEffect to the parent component. Signed-off-by: dbczumar <corey.zumar@databricks.com>

Update test to use renamed query param key 'selectedDatasetId'

fd94a22

Signed-off-by: dbczumar <corey.zumar@databricks.com>

BenWilson2 added v3.7.1 and removed v3.7.0 labels Dec 4, 2025

daniellok-db approved these changes Dec 4, 2025

View reviewed changes

dbczumar added this pull request to the merge queue Dec 4, 2025

Merged via the queue into mlflow:master with commit 24c1353 Dec 4, 2025
56 of 59 checks passed

dbczumar deleted the dataset_url branch December 4, 2025 23:40

	const QUERY_PARAM_KEY = 'selectedDataset';
	const QUERY_PARAM_KEY = 'selectedDatasetId';

-  const [selectedDatasetId, setSelectedDatasetId] = useSelectedDatasetBySearchParam();
+  const [selectedDatasetId, setSelectedDatasetId] = useSelectedDatasetBySearchParam();
+  const onSelectDataset = useCallback((dataset) => {
+    setSelectedDataset(dataset);
+    setSelectedDatasetId(dataset.dataset_id);
+  });

		// Sync dataset selection state with URL and available datasets
		useEffect(() => {

Conversation

dbczumar commented Dec 1, 2025 • edited by github-actions bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Install mlflow from this PR

Related Issues/PRs

What changes are proposed in this pull request?

How is this PR tested?

Does this PR require documentation update?

Release Notes

Is this a user-facing change?

What component(s), interfaces, languages, and integrations does this PR affect?

How should the PR be classified in the release notes? Choose one:

Should this PR be included in the next patch release?

Uh oh!

github-actions bot commented Dec 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dbczumar Dec 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daniellok-db left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

dbczumar commented Dec 1, 2025 •

edited by github-actions bot

Loading

github-actions bot commented Dec 1, 2025 •

edited

Loading

dbczumar Dec 1, 2025 •

edited

Loading