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

cml ci: unshallow repos #956

Closed
wants to merge 1 commit into from
Closed

Conversation

pmrowla
Copy link

@pmrowla pmrowla commented Apr 13, 2022

Docs for cml ci state

Prepares Git repository for CML operations (setting Git user.name & user.email, unshallow clone and undo CI oddities such as origin URL formatting and HTTP remote proxies).

but cml ci does not currently unshallow the clone. It just does fetch --all, which fetches all remote branches, but does not actually modify fetch depth of any of those branches. So in github/gitlab the repo will just have a bunch of shallow branches with depth=1 rather than a single ref/commit with depth=1.

Assuming that unshallowing all clones by default is the intended behavior (which I'm not sure is actually desired, since it will have a significant performance impact in large git repos), what cml ci needs to be doing is something like

# check if the repo is shallow
if [ -f `git rev-parse --git-dir`/shallow ]; then
    git fetch --unshallow
fi
git fetch --all

Note that the check for GIT_DIR/shallow is required since repeated git fetch --unshallow calls will fail if the repo is not actually shallow, which would also cause repeated cml ci calls to fail

(Also, I have not tested the changes in this PR, and I'm not a node/js dev so there's probably a better way of accomplishing this in node)

@0x2b3bfa0 0x2b3bfa0 requested a review from a team April 13, 2022 06:19
@0x2b3bfa0 0x2b3bfa0 added enhancement New feature or request cml-ci Subcommand labels Apr 13, 2022
@pmrowla pmrowla removed their assignment Apr 13, 2022
@pmrowla
Copy link
Author

pmrowla commented Apr 13, 2022

as discussed with @casperdcl, unshallowing should probably not be the default behavior in cml ci, it would be better to have a separate flag for unshallowing, and it should be documented accordingly, this PR can be closed as needed by the cml team

the main issue here is that "use cml ci" is currently being suggested to users as a fix for not being able to run dvc exp run inside CML due to git merge being unsupported in shallow clones (depending on specific fetch depth), but cml ci does not actually resolve the issue right now (and users still need to explicitly set fetch-depth: 0 in the github/gitlab checkout action)

@casperdcl
Copy link
Contributor

Thanks @pmrowla! Just opened #957 simultaneously, woops.

@pmrowla pmrowla closed this Apr 13, 2022
@pmrowla pmrowla deleted the ci-unshallow branch April 13, 2022 06:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cml-ci Subcommand enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants