Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

dvc exp remove --keep #10593

Closed
majidaldo opened this issue Oct 18, 2024 · 5 comments
Closed

dvc exp remove --keep #10593

majidaldo opened this issue Oct 18, 2024 · 5 comments
Labels
A: experiments Related to dvc exp feature request Requesting a new feature good first issue p2-medium Medium priority, should be done, but less important

Comments

@majidaldo
Copy link

I don't understand why one would want to remove the last n exps. Shouldn't this be in terms of --keep last n? The typical case is that one wants to just remove old exps.

@shcheklein shcheklein added feature request Requesting a new feature A: experiments Related to dvc exp good first issue p2-medium Medium priority, should be done, but less important labels Oct 19, 2024
@shcheklein
Copy link
Member

Makes sense to me to have this as an option.

@majidaldo
Copy link
Author

I don't understand why one would want to remove the last n exps. Shouldn't this be in terms of --keep last n? The typical case is that one wants to just remove old exps.

Related to this is that dvc exp list -A should be ordered according to the history. Right now the base commits are ordered alphabetically.

@rmic
Copy link
Contributor

rmic commented Nov 22, 2024

Hi there !

Correct me if I'm wrong here (I'm just starting with this codebase).

The idea here could be, first sort the commits in chronological order in _get_n_commits (

dvc/dvc/scm.py

Line 182 in fb24775

def _get_n_commits(scm: "Git", revs: list[str], num: int) -> list[str]:
then add an argument to to indicate the direction (before or after) in which we'd like to search the given number of commits, that parameter would drip down from
exp_ref_dict = _resolve_exp_by_baseline(repo, rev, num, git_remote)

and then add arguments to the exp remove cli command telling if we want to delete --num experiments either --before (new param) or --after (new param) the --rev commit

If I'm not too mistaken, I'm interested in giving it a shot as a first contribution to this repo ! Let me know 😊

@majidaldo
Copy link
Author

Hi there !

Correct me if I'm wrong here (I'm just starting with this codebase).

The idea here could be, first sort the commits in chronological order in _get_n_commits (

dvc/dvc/scm.py

Line 182 in fb24775

def _get_n_commits(scm: "Git", revs: list[str], num: int) -> list[str]:
then add an argument to to indicate the direction (before or after) in which we'd like to search the given number of commits, that parameter would drip down from
dvc/dvc/repo/experiments/remove.py

Line 66 in fb24775

exp_ref_dict = _resolve_exp_by_baseline(repo, rev, num, git_remote)
and then add arguments to the exp remove cli command telling if we want to delete --num experiments either --before (new param) or --after (new param) the --rev commit

If I'm not too mistaken, I'm interested in giving it a shot as a first contribution to this repo ! Let me know 😊

ya overall that sounds right. thanks for tackling this.

re: chronological. i thought, more specifically, it would just be going up the commit tree but i understand it would be the same ordering ...typically.

@rmic
Copy link
Contributor

rmic commented Nov 23, 2024

Fixing the order of the commits was not required for the --keep thing, which also explains why I finally did not go with the --before and --after params on the CLI as initially thought.

Yet, fixing the order of dvc exp list is something I might enjoy working next.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
A: experiments Related to dvc exp feature request Requesting a new feature good first issue p2-medium Medium priority, should be done, but less important
Projects
None yet
Development

No branches or pull requests

3 participants