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

experiments: auto push experiments #10323

Merged
merged 4 commits into from
Mar 4, 2024
Merged

experiments: auto push experiments #10323

merged 4 commits into from
Mar 4, 2024

Conversation

AlexandreKempf
Copy link
Contributor

@AlexandreKempf AlexandreKempf commented Feb 26, 2024

As requested by #10137, we want a configuration parameter to auto-push the experiments once they are run. Auto-push means the experiments and artifacts will be sent to git and the DVC remote after each experiment succeeds.

This PR also introduces a default for the git_remote to origin

⚠️ @mattseddon In this PR I changed the DVC config (.dvc/config) to add the new field exp.auto_push. When I'm testing to run a demo project locally and look at my studio dashboard (from the web, not a branch), I have a warning
image. Do you know If the warning would disappear if I build the studio dashboard with the DVC code from this PR? Or should I change code on the studio backend repo to update the new config with the new field exp.auto_push?

Thank you for the contribution - we'll try to review it as soon as possible. 🙏

Copy link

codecov bot commented Feb 26, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.38%. Comparing base (3b2e031) to head (40692a6).

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #10323      +/-   ##
==========================================
- Coverage   90.59%   90.38%   -0.21%     
==========================================
  Files         500      500              
  Lines       38587    38614      +27     
  Branches     5573     5581       +8     
==========================================
- Hits        34959    34903      -56     
- Misses       2983     3040      +57     
- Partials      645      671      +26     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mattseddon
Copy link
Member

mattseddon commented Feb 26, 2024

@mattseddon In this PR I changed the DVC config (.dvc/config) to add the new field exp.auto_push. When I'm testing to run a demo project locally and look at my studio dashboard (from the web, not a branch), I have a warning

I am not sure, I would try building Studio locally with the version of DVC from this branch. You can set dvc config studio.url http://localhost:8111 in order to push experiments to the local version.

Edit: are you sure you had the version of DVC from this PR installed into your virtual environment?

@mattseddon
Copy link
Member

Studio loads the config here: https://github.com/iterative/studio/blob/bb120fcd4899ca43b33db4609aa6c4e321931660/backend/repos/parsing/__init__.py#L197. I would assume that because the version in DVC does not have the required config key that it is throwing the expected error. You should be able to prove that by writing a test. If all this is true then it will be slightly tricky to release this feature.

@AlexandreKempf
Copy link
Contributor Author

@mattseddon So I realized that it is only doing it when I set the exp.auto_push in my local config (the one for the project). When it is set globally: dvc config --global exp.auto_push true, it works perfectly.

I didn't manage to run studio on my computer despite my best efforts. All containers seem to work well except the UI one. Might be the container or my 3000 port that was already used and that I didn't kill properly. I might ask you or Jelle for some help to figure out what is wrong. I would love to have it running to do some baby PR there and get familiar with the code.

From your last comment, It seems that Studio is using DVC to load and check the config, so if it works with DVC it should work with studio (I'll ask the person in charge of the next DVC release to ping me so that I can do a PR in studio to update dvc version).

I have no idea how to start the integration tests. Do you mind helping me on that one? Or pointing me some code I should look at to get inspired? What would you test that is not already tested?

Thanks for your replies :D

@AlexandreKempf AlexandreKempf marked this pull request as ready for review February 28, 2024 18:32
@shcheklein
Copy link
Member

@dberenbaum can we merge this and do a release?

@skshetry skshetry self-assigned this Mar 1, 2024
@@ -701,13 +689,30 @@ def _auto_push(
@classmethod
def commit(
Copy link
Member

Choose a reason for hiding this comment

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

commit() might not be a good place to auto_push.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@skshetry Do you want to take it over?

Copy link
Member

@skshetry skshetry Mar 1, 2024

Choose a reason for hiding this comment

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

yeah, I have assigned it myself, and will get to it next week.

@skshetry skshetry force-pushed the auto-push-experiments branch from ff736cc to 40692a6 Compare March 4, 2024 08:28
@skshetry skshetry merged commit cd1e228 into main Mar 4, 2024
18 of 19 checks passed
@skshetry skshetry deleted the auto-push-experiments branch March 4, 2024 08:35
@skshetry
Copy link
Member

skshetry commented Mar 4, 2024

Thanks @AlexandreKempf. 🙂

BradyJ27 pushed a commit to BradyJ27/dvc that referenced this pull request Apr 22, 2024
* add auto_push for experiment on run and on save.
add config.exp.auto_push and config.exp.git_remote

* add func tests

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* separate auto_push behaviour out of commit

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Saugat Pachhai (सौगात) <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants