Skip to content

Conversation

@skshetry
Copy link
Collaborator

This PR splits scm-related exceptions into two kinds:

  1. Internal scm exceptions that are in dvc.scm.exceptions and likely going to be available as scmrepo.exceptions.
  2. External DVC-related exceptions in dvc.scm.base that is required for UI and dvc's API to work.
    This will be available in dvc.scm when we migrate.

So, due to 2, there are some exceptions with the same name in both 1 and 2, but they serve a different purpose and are just reraised from one to another.

This PR is still incomplete, in that we have to decide what to do with raised SCMError exceptions. As DVC is unaware of these exceptions, places where we are checking for DvcException or where we just exit from the main, it will throw errors with:

Having any troubles? Hit us up at https://dvc.org/support, we are always happy to help!

There may definitely be some UI issues with this breakage, which we need to discuss. The easiest path forward that I see is to make these changes and fix them in the future as we find them.

The other way would require us to go through chains of API calls where we raise dvc.scm.exceptions.SCMError and catch those and reraise those as dvc.scm.base.SCMError.

Copy link
Contributor

Choose a reason for hiding this comment

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

Not related to this PR specifically, but we will probably need a dvc.utils.resolve_rev function that wraps the separate SCM package's resolve_rev.

The existing scm.git.resolve_rev method contains DVC specific behavior, where we extend the standard git ref (branch/tag) resolution to also include refs in the exps namespace. This should probably stay in DVC (and not the standalone package).

scm.git.__init__.py::resolve_rev

Copy link
Collaborator Author

@skshetry skshetry Nov 23, 2021

Choose a reason for hiding this comment

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

@pmrowla, this dvc/scm/__init__.py will remain as dvc/scm.py. So, this resolve_rev is the one that will provide DVC specific behaviour. The following contents will be moved here in successive PRs:

https://github.com/iterative/dvc/blob/644e9ede924b6416b4ff714fdc0c3463190ab6f2/dvc/scm/git/__init__.py#L311-L318

@efiop
Copy link
Contributor

efiop commented Nov 23, 2021

@skshetry could you rebase, please? (sorry, I merged #7026 before this one which resulted in conflicts 🙁)

@skshetry
Copy link
Collaborator Author

The new changes remaps internal SCMError into DvcException counterparts in external_repo and experiments (as @pmrowla suggested).

@efiop efiop merged commit d95dd27 into treeverse:master Nov 23, 2021
@skshetry skshetry deleted the scm-refactor branch November 23, 2021 17:24
@efiop efiop added the refactoring Factoring and re-factoring label Nov 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

refactoring Factoring and re-factoring

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants