Skip to content

Conversation

skshetry
Copy link
Collaborator

@skshetry skshetry commented Jun 2, 2025

See #10700 for the motivation.

We need to rename the url to something else for fsspec.core.url_to_fs to work for dvc urls with storage_options.

We have two options to consider:

  1. Repurpose the existing repo= argument to accept both str and Repo instances, or
  2. Rename url= to something else (preferably repo), and introduce a new argument for accepting Repo instances.

Most of the API in dvc.api already uses repo= (as a str path to the DVC/Git repository, eg: dvc.api.open and dvc.api.read. So, I prefer the 1st option, as it will be a good opportunity to align naming and use repo= consistently.

But I am happy with either of the options.

Also note that accepting Repo instance is part of an internal API, and is done for avoiding cost of opening another DVC/Git repository.

Related:

@skshetry skshetry added this to DVC Jun 3, 2025
@skshetry skshetry moved this to Done in DVC Jun 3, 2025
@skshetry skshetry moved this from Done to In Progress in DVC Jun 3, 2025
@skshetry skshetry force-pushed the fix-10700 branch 2 times, most recently from 9917b36 to 044cc96 Compare June 3, 2025 06:25
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR renames the argument "url" to "repo" for the DVCFileSystem, improving the clarity of parameter names.

  • Updates tests to construct DVCFileSystem using the new "repo" parameter where possible while still supporting backward compatibility.
  • Adjusts the init signature and deprecation fallback in dvc/fs/dvc.py.
  • Updates dependency handling in dvc/dependency/repo.py to use the new argument name.

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

File Description
tests/unit/fs/test_dvcfs.py Updated DVCFileSystem instantiation to remove deprecated "url" usage.
tests/unit/fs/test_dvc.py Updated parameterized tests to cover both "repo" and legacy "url".
dvc/fs/dvc.py Modified init to prioritize the new "repo" argument and handle fallback.
dvc/dependency/repo.py Adjusted dependency creation to pass "repo" instead of "url".
Comments suppressed due to low confidence (1)

tests/unit/fs/test_dvcfs.py:781

  • Consider updating the test to explicitly use the new 'repo' parameter, for example, as a positional argument or named 'repo', to align with the updated API design and improve clarity.
fs = DVCFileSystem(url=tmp_dir)

@skshetry skshetry requested a review from shcheklein June 3, 2025 06:31
@skshetry skshetry marked this pull request as ready for review June 3, 2025 06:36
@skshetry
Copy link
Collaborator Author

skshetry commented Jun 3, 2025

Failures are unrelated and is likely due to a new pytest release. Will take a look separately.

@skshetry skshetry self-assigned this Jun 3, 2025
@skshetry skshetry moved this from In Progress to Review In Progress in DVC Jun 3, 2025
@skshetry skshetry merged commit e04f149 into main Jun 5, 2025
32 of 40 checks passed
@skshetry skshetry deleted the fix-10700 branch June 5, 2025 02:45
@github-project-automation github-project-automation bot moved this from Review In Progress to Done in DVC Jun 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

1 participant