dvc exp run --set-param does not change parameter values from a Python dict

[a-rostek]

Bug Report

dvc exp run --set-param does not change parameter values from a Python dict

Description

When trying to do dvc exp run --set-param my_params.py:my_dict.my_key=3 the parameter value is not changed to 3, it remains the same as previously. The output is:

Stage 'my_stage' didn't change, skipping                                                             

Reproduce

1. git init
2. dvc init
3. dvc.yaml

stages:
  my_stage:
    cmd: cat a.txt > b.txt
    deps:
    - a.txt
    outs:
    - b.txt
    params:
    - my_params.py:
      - my_dict

4. my_params.py

my_dict = {
    'my_key': 2
}

5. echo dummyString > a.txt
6. dvc exp run --set-param my_params.py:my_dict.my_key=3

Expected

I would expect this output (which can be achieved by changing the value of my_key manually in my_params.py and then running dvc exp run):

Running stage 'my_stage':                                                                            
> cat a.txt > b.txt
Updating lock file 'dvc.lock'                                                                        

To track the changes with git, run:

	git add dvc.yaml a.txt dvc.lock my_params.py
                                                                      
Reproduced experiment(s): exp-e60e4
Experiment results have been applied to your workspace.

To promote an experiment to a Git branch run:

	dvc exp branch <exp> <branch>

Environment information

Output of dvc doctor:

$ dvc doctor
DVC version: 2.8.3 (pip)
---------------------------------
Platform: Python 3.8.10 on Linux-5.11.0-40-generic-x86_64-with-glibc2.29
Supports:
	webhdfs (fsspec = 2021.10.1),
	http (aiohttp = 3.7.4.post0, aiohttp-retry = 2.4.5),
	https (aiohttp = 3.7.4.post0, aiohttp-retry = 2.4.5),
	s3 (s3fs = 2021.8.1, boto3 = 1.17.106)
Cache types: reflink, hardlink, symlink
Cache directory: xfs on /dev/sdb
Caches: local
Remotes: None
Workspace directory: xfs on /dev/sdb
Repo: dvc, git

[daavoo]

I can reproduce

[daavoo]

There is an issue with how we update the python values:

dvc/dvc/utils/serialize/_py.py

Lines 57 to 59 in 3143975

	lines[lineno] = lines[lineno].replace(
	f" = {old_dct[key]['value']}", f" = {value}"
	)

Wich only match assignments following that string pattern (" = foo")

[skshetry]

We can only have basic write support for now (see #5777). Proper write support will require us to track segments of the source code, which gets complicated to support for dict/list inner items, but can be done. I attempted this on #5778, see #5778 (comment).

Unless we change anything internally, we have to live with these bugs.
Currently, Python as params is only meant to be used for simple things. Using other types of parameters files is recommended.

[daavoo]

We can only have basic write support for now (see #5777). Proper write support will require us to track segments of the source code, which gets complicated to support for dict/list inner items, but can be done. I attempted this on #5778, see #5778 (comment).

Unless we change anything internally, we have to live with these bugs. Currently, Python as params is only meant to be used for simple things. Using other types of parameters files is recommended.

Makes sense. However, we could improve some UX interactions and/or remark this on the docs side.

We are currently just ignoring the errors from params that we can't parse, which can cause some crashes afterward and/or unexpected behavior (like this issue).

Perhaps we could just raise errors on unparsed params. Alternatively, we could try to collect the params that we failed to parse and check if they are actually dependencies or args to --set-params and fail only in that cases.

[dberenbaum]

I would expect from #5477 that if the parameter cannot be replaced, then dvc should fail, stating that the parameter cannot be found.

[daavoo]

I would expect from #5477 that if the parameter cannot be replaced, then dvc should fail, stating that the parameter cannot be found.

That issue / P.R. was more about missing / incorrectly parsed params. The validation happens before the replacement.

We should probably validate after replacement (although afaik this is only an issue with Python format where the replacement is very hacky, as the snippet linked above).

[dberenbaum]

So dvc validates that my_params.py:my_dict.my_key exists but can't replace it? In that case, I agree that we should probably raise an error to say something like Unable to parse {file} to set parameter {key}. Please update {file} instead of using --set-param.

[dberenbaum]

Ultimately, it seems like other methods for logging parameters, like those discussed in #6506, may be a more useful approach than parsing/replacing parameters from Python files.