-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
params: untracked values in params files are not ignored in params/exp commands #5451
Comments
Can you run this command and post the output:
|
I see, Here is the output of the command: Path Param Old New
data/specs/bert-train-no-empty-answers_experiment.json command ['./src/processing/run.sh', 'data/specs/bert-train-no-empty-answers_experiment.json'] ['./src/processing/run.sh', 'data/specs/bert-train-no-empty-answers_experiment.json']
data/specs/bert-train-no-empty-answers_experiment.json experiment_dir bert-train-no-empty-answers bert-train-no-empty-answers
data/specs/bert-train-no-empty-answers_experiment.json experiment_name bert-train-no-empty-answers bert-train-no-empty-answers
data/specs/bert-train-no-empty-answers_experiment.json inputs ['/home/gb/Documents/Research/quail-experiments/data/quail_no_empty_answers/train.json', '/home/gb/Documents/Research/quail-experiments/data/quail_no_empty_answers/dev.json', '/home/gb/Documents/Research/quail-experiments/data/models/bert-base-uncased'] ['/home/gb/Documents/Research/quail-experiments/data/quail_no_empty_answers/train.json', '/home/gb/Documents/Research/quail-experiments/data/quail_no_empty_answers/dev.json', '/home/gb/Documents/Research/quail-experiments/data/models/bert-base-uncased']
data/specs/bert-train-no-empty-answers_experiment.json metrics ['train_metrics.json', 'eval_metrics.json'] ['train_metrics.json', 'eval_metrics.json']
data/specs/bert-train-no-empty-answers_experiment.json outputs ['data/models/bert-train-no-empty-answers/config.json', 'data/models/bert-train-no-empty-answers/pytorch_model.bin', 'data/models/bert-train-no-empty-answers/special_tokens_map.json', 'data/models/bert-train-no-empty-answers/tokenizer_config.json', 'data/models/bert-train-no-empty-answers/training_args.bin', 'data/models/bert-train-no-empty-answers/vocab.txt'] ['data/models/bert-train-no-empty-answers/config.json', 'data/models/bert-train-no-empty-answers/pytorch_model.bin', 'data/models/bert-train-no-empty-answers/special_tokens_map.json', 'data/models/bert-train-no-empty-answers/tokenizer_config.json', 'data/models/bert-train-no-empty-answers/training_args.bin', 'data/models/bert-train-no-empty-answers/vocab.txt']
data/specs/bert-train-no-empty-answers_experiment.json params.cache_dir /tmp /tmp
data/specs/bert-train-no-empty-answers_experiment.json params.data_dir data/quail_no_empty_answers data/quail_no_empty_answers
data/specs/bert-train-no-empty-answers_experiment.json params.do_eval True True
data/specs/bert-train-no-empty-answers_experiment.json params.do_train True True
data/specs/bert-train-no-empty-answers_experiment.json params.fp16 True True
data/specs/bert-train-no-empty-answers_experiment.json params.fp16_opt_level O1 O1
data/specs/bert-train-no-empty-answers_experiment.json params.gradient_accumulation_steps 8 8
data/specs/bert-train-no-empty-answers_experiment.json params.learning_rate 5e-05 5e-05
data/specs/bert-train-no-empty-answers_experiment.json params.max_seq_length 484 484
data/specs/bert-train-no-empty-answers_experiment.json params.meta bert-train-no-empty-answers bert-train-no-empty-answers
data/specs/bert-train-no-empty-answers_experiment.json params.metrics_dir data/metrics/bert-train-no-empty-answers data/metrics/bert-train-no-empty-answers
data/specs/bert-train-no-empty-answers_experiment.json params.model_name_or_path data/models/bert-base-uncased data/models/bert-base-uncased
data/specs/bert-train-no-empty-answers_experiment.json params.model_type bert bert
data/specs/bert-train-no-empty-answers_experiment.json params.num_train_epochs 2 2
data/specs/bert-train-no-empty-answers_experiment.json params.output_dir data/models/bert-train-no-empty-answers data/models/bert-train-no-empty-answers
data/specs/bert-train-no-empty-answers_experiment.json params.per_device_eval_batch_size 8 8
data/specs/bert-train-no-empty-answers_experiment.json params.per_device_train_batch_size 2 2
data/specs/bert-train-no-empty-answers_experiment.json params.results_dir data/results/bert-train-no-empty-answers data/results/bert-train-no-empty-answers
data/specs/bert-train-no-empty-answers_experiment.json params.save_logits True True
data/specs/bert-train-no-empty-answers_experiment.json params.save_steps 0 0
data/specs/bert-train-no-empty-answers_experiment.json params.save_total_limit 0 0
data/specs/bert-train-no-empty-answers_experiment.json params.task_name generic generic
data/specs/bert-train-no-empty-answers_experiment.json params.warmup_steps 500 500
data/specs/bert-train-no-empty-answers_experiment.json qualified_name bert-train-no-empty-answers bert-train-no-empty-answers
data/specs/bert-train-no-empty-answers_experiment.json results ['train_predictions.json', 'train_nbest_predictions.json', 'eval_predictions.json', 'eval_nbest_predictions.json'] ['train_predictions.json', 'train_nbest_predictions.json', 'eval_predictions.json', 'eval_nbest_predictions.json']
data/specs/bert-train-no-empty-answers_experiment.json scripts ['/home/gb/Documents/Research/quail-experiments/src/processing/run.sh'] ['/home/gb/Documents/Research/quail-experiments/src/processing/run.sh']
params.yaml classification.autogoal False False
params.yaml classification.balanced False False
params.yaml classification.early_stop 6 6
params.yaml classification.iterations 200 200
params.yaml classification.memory 64 64
params.yaml classification.metric weighted_f1 weighted_f1
params.yaml classification.multi_layer.batch_size 200 200
params.yaml classification.multi_layer.epochs 100 100
params.yaml classification.multi_layer.lr 0.001 0.001
params.yaml classification.pipeline logreg logreg
params.yaml classification.popsize 50 50
params.yaml classification.selection 10 10
params.yaml classification.sweep_features False False
params.yaml classification.test_size 0.2 0.2
params.yaml features.context False False
params.yaml features.embeddings False False
params.yaml features.endings False False
params.yaml features.logits False False
params.yaml features.normalization False False
params.yaml features.oversample False False
params.yaml features.question False False
params.yaml features.text_length False False Coming from |
Can be reproduced with an updated version of def test_pipeline_tracked_params(tmp_dir, scm, dvc, run_copy):
from dvc.dvcfile import PIPELINE_FILE
tmp_dir.gen(
{"foo": "foo", "params.yaml": "foo: bar\nxyz: val\nabc: ignore"}
)
run_copy("foo", "bar", name="copy-foo-bar", params=["foo,xyz"])
scm.add(["params.yaml", PIPELINE_FILE])
scm.commit("add stage")
tmp_dir.scm_gen(
"params.yaml", "foo: baz\nxyz: val\nabc: ignore", commit="baz"
)
tmp_dir.scm_gen(
"params.yaml", "foo: qux\nxyz: val\nabc: ignore", commit="qux"
)
assert dvc.params.show(revs=["master"]) == {
"master": {"params.yaml": {"foo": "qux", "xyz": "val"}}
}
|
Hoou, it seems more critical than I thought, can I help in anyway? |
@geblanco we should be able to get to it in our next sprint, thanks for offering though! |
Okay, great!! :) |
Are we trying to change the behaviour here for 2.0? Not sure why we are treating it as a bug, it's expected/intended (though not intuitive). 🙂 |
@skshetry What is the reasoning behind having this as the intended behavior? |
I am not aware of the actual reasoning, but this decision was made when params were introduced in 1.0, so it was probably just a simplification. I did push for changing this behaviour in 2.0, as it makes a feature of auto-tracking parameterized values useless (I also treated this as a bug till the end 😢). But it was decided to change it after 2.0 or introduce separate flags for this behaviour. cc'ing @dmpetrov about this discussion. |
It seems like it should be changed to me, or at the very least our internal |
I don't know if it was a feature or a bug, but from my user perspective, it's annoying to get parameters into |
Blocked by #5539 |
@pmrowla |
I think that is the point. I am unable to come up with a use case where we need to |
@pared filtering the params afterwards makes sense to me, |
There are a couple of questions if we are going to change the default:
|
To get the current behavior (meaning, include untracked values in DVC-tracked params dependencies), what about just With
but do we know many DVC users are actually using it this way? If it's not actually a common use case, I think it's reasonable to just require both |
@pmrowla, Maybe introduce |
@geblanco Thanks for the detailed info! What would you want to see in
For the last question, I'm thinking that multiple pipelines might be needed at times with separate parameters in each. Should dvc include only the parameters tracked by the current pipeline? |
Thanks to you all for this great software you're crafting! @dberenbaum I'll try to answer inline Yes effectively, it seems natural to me to only take into account parameters that have been explicitly tracked down (
When thinking about diffing parameters, I think more in terms of tracked parameters and their values, rather than the source those came from. So I guess it would be great to get the diff from all tracked parameters, regardless of its origin (as is done right now). Also, as the path of each tracked parameter is showed, filtering the output based on file is as easy as
The last two questions are very interesting in terms of researching methodology, I'm currently trying to find my way through it. Right now, what I usually do regarding the classification step is:
When trying new classifiers, sometimes more parameters are required, in such scenario I just modify the current classification stage within DVC to track my new parameters, though it does not seem to be the best procedure. In some cases, the code for a new classifier is really different from the previous one (i.e.: going from a RegressionTree to a whole transformers-based model), in those cases, the entire set of parameters might be changed, and it makes a lot sense to separate both source code files, and probably, add a different DVC stage, splitting the pipeline from the previous stage (the one before classification), this yields a tree with two nodes that are intended for the same purpose: classify previously processed data. The problem here, is that then, you have to point the output results of both stages to separate files (to avoid output conflicts), if we later add a stage that processes those results (joining both classification nodes) then, it has to depend on both results files, and so on... That is why I always end up modifying the DVC stage.
Yes, that makes a lot of sense too. When a pipeline is split into two different nodes, and each node tracks different parameters (and probably continue evolving separately) it would be great to track those separately too. Otherwise, parameters would get tangled and it becomes harder to see the progress of each pipeline. For this scenario, maybe it would be possible to get a clearer idea with some use cases (I add one at the end, does it makes sense?)
In this case, it would make sense to track parameters for the first pipeline separately from the second one, as both serve for different purposes and can evolve within different timelapses. |
@pared @pmrowla @skshetry My thoughts based on this feedback (which mostly match what I would expect):
|
So in order to introduce the change for As @efiop mentioned in here params and metrics are created based on assumptions introduced as we were developing it, sometimes by user. Current state might be confusing. In order not to break the current version of DVC, i think we should go with the opposite of |
Okay, looks like this keeps getting more complicated! Maybe we jumped too far ahead here. Are there any options for 2.x that won't need to be reverted in 3.0? Can IMO, this still needs more thought about how to only see parameters relevant to a particular experiment/pipeline, but that can be addressed for 3.0. |
I guess one workaround would be for the user to add another level of nesting in the params file so that you could do something like |
Okay, let's go ahead with this plan and add it to |
By "DVC-tracked params", it seems we really mean "parameter dependencies" (in stage definitions). So maybe But if this should also apply to metrics then "deps" isn't that meaningful. Could also add To encompasses both I can think of |
I don't think we should be concerned here with any |
What's the status of the short-term fix here? I thought that the functionality in #5541 was basically okay except that it shouldn't be the default. Is that right? |
@dberenbaum the fix is in #5550 I am still working on that |
Good questions on
Sounds like a rebrand of
Agree! Is that what iterative/enhancement-proposals/pull/1 is for? |
This issue is hopefully going to be addressed by #5550. That's going to be a pretty limited change to provide an option to exclude untracked parameters. iterative/enhancement-proposals/pull/1 is for related changes building off of this discussion but less focused on fixing the specific issue here. Basically, we're trying to keep the issue narrowly focused and use the enhacement proposal for the broader changes we want to make. |
@geblanco |
Bug Report
exp show: Invalid or wrong values for column names and values
Description
I have an experiment trying different parameters, it is the last stage of the pipeline, and the only with checkpointed outputs. Following this procedure: run the experiment (
dvc exp run
), change a parameter inparams.yaml
and run it again, do it several times. To visualize the results I rundvc exp show
, but I get wrong column names and values.I have a doubt about the intended behavior of this feature, when
showing
experiments, the idea is to show the comparison of metrics along parameters. It is expected to show the difference in metrics for ALL stages that contain metrics and parameters, or only those being experimented with (the ones that are run when iterating experiments)?Trying to hide the trash columns with
--exclude-metrics
allows me to remove some columns, though some others are not recognized. But, in any case, it doesn't remove the strange field values. I include a photo to avoid breaking the format:Reproduce
Reproducing is a bit computationally expensive, it requires training a bert model and doing some operations on it's embeddings, it can be accomplished in the following way (at least one GPU with enough RAM, 5-8hrs of computations).
Expected
The command should show a table with the parameters tracked by the stage, it's changes and results in metrics.
Environment information
Output of
dvc version
:Additional Information (if any):
I have the intuition that it has to do with the parsing of parameters from a file other than
params.yaml
. To be more clear, here is the output in json of theshow
command:There are various entries for
data/specs/bert-train-no-empty-answers_experiment.json
stated as parameters, with the whole file contents as parameters, although the stage using that file only uses a part of it as parameters (see stage bert-train-no-empty-answers indvc.yaml
below)dvc exp show --show-json
I hope this information is useful enough, but, if anything is needed just let me know!
The text was updated successfully, but these errors were encountered: