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

exp init: don't add outs to .gitignore #7740

Closed
wants to merge 1 commit into from
Closed

Conversation

dberenbaum
Copy link
Contributor

Addresses #5802 (comment) by not adding to .gitignore during exp init since it breaks for subdirectories. Adding to .gitignore is still done on exp run / repro.

Copy link
Contributor

@daavoo daavoo left a comment

Choose a reason for hiding this comment

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

Out of the scope of the P.R. but was wondering why exp init doesn't use stage.add?

Copy link
Contributor

@daavoo daavoo left a comment

Choose a reason for hiding this comment

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

Need to update tests:

assert scm.is_tracked(".gitignore")

@dberenbaum
Copy link
Contributor Author

Need to update tests:

Sorry, forgot to remove those lines before before pushing. It should pass now.

Out of the scope of the P.R. but was wondering why exp init doesn't use stage.add?

@skshetry can answer, but I'm guessing from

with _disable_logging(), repo.scm_context(autostage=True, quiet=True):
that it's because we need to modify some things like disabling output.

@skshetry
Copy link
Member

Out of the scope of the P.R. but was wondering why exp init doesn't use stage.add?

Previously, we used to ask for confirmation in an interactive mode before creating the stage. exp init also auto-stages dvc.yaml and params.yaml file which stage add does not do by default (unless core.autostage=true).

Now, this PR is another reason for not using stage add. stage add also auto-commits the file, whereas dvc exp init may require more processing before committing the file (if not now, in the future anyway), so I don't think stage add is a good fit.

We could extend stage add for sure, but dumping stages are complex and can be done in a lot of ways, I don't want to overcomplicate it with all the complexities that may be introduced by exp init.

@dberenbaum dberenbaum requested a review from daavoo May 16, 2022 12:39
@dberenbaum
Copy link
Contributor Author

I'm fine with keeping the scope of this PR as small as possible for now. Could one of you please review now that the tests have been fixed?

@skshetry
Copy link
Member

@dberenbaum, I am a bit hesitant to change the behaviour here, although no strong opinion. I'm okay with:

  1. either git-ignoring what we can and leave the rest to repro,
  2. git-ignoring by traversing upwards (it's going to require some changes in scmrepo, so definitely not a straightforward fix),
  3. or, generate intermediate directories.

DVC expects the path to the output to exist in a lot of places (more in relation to stage's working directory, but usually that's where .dvc files are created by default). WDYT of 1 and 3, or a combination of both options?

@dberenbaum
Copy link
Contributor Author

@skshetry Can you explain why you are hesitant (and why dvc should generate .gitignore files at all when creating stages)?

I opened a PR for 3 in #7751. That would solve the immediate issue, although the rationale for adding to .gitignore during stage creation is still unclear to me.

@skshetry
Copy link
Member

skshetry commented Jun 7, 2022

@skshetry Can you explain why you are hesitant (and why dvc should generate .gitignore files at all when creating stages)?

I opened a PR for 3 in #7751. That would solve the immediate issue, although the rationale for adding to .gitignore during stage creation is still unclear to me.

The only concern is that the users may git add accidentally in between, the .gitignore prevents that.

@dberenbaum
Copy link
Contributor Author

Good point @skshetry. I'll close this one for now since the issue was addressed in #7752. We can come back to it if needed.

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

Successfully merging this pull request may close these issues.

None yet

3 participants