`params`: Python parameters file does not support type constructors

[daavoo]

Bug Report

Description

When using Python parameters file, the type constructors syntax (i.e. my_dict = dict(a=1)) is not correctly parsed.

Reproduce

from pathlib import Path
from dvc.utils.serialize import load_py

Path("params.py").write_text("my_dict = dict(a=1)")

parsed_params = load_py("params.py")

# parsed_params is empty
# Raises KeyError: 'my_dict' 
assert parsed_params["my_dict"]["a"] == 1

Expected

I would expect type constructors to be correctly parsed, like the other syntaxes for constructing python built-in types. The following works as intended:

from pathlib import Path
from dvc.utils.serialize import load_py

Path("params.py").write_text("my_dict = {'a': 1}")

parsed_params = load_py("params.py")

assert parsed_params["my_dict"]["a"] == 1

Environment information

Output of dvc doctor:

$ dvc doctor
DVC version: 2.6.4+b5fe9b.mod 
---------------------------------
Platform: Python 3.9.6 on macOS-10.16-x86_64-i386-64bit
Supports:
        azure (adlfs = 2021.7.1, knack = 0.8.2, azure-identity = 1.6.0),
        gdrive (pydrive2 = 1.8.3),
        gs (gcsfs = 2021.7.0),
        hdfs (pyarrow = 5.0.0),
        webhdfs (hdfs = 2.5.8),
        http (requests = 2.26.0),
        https (requests = 2.26.0),
        s3 (s3fs = 2021.7.0, boto3 = 1.17.106),
        ssh (sshfs = 2021.7.1),
        oss (ossfs = 2021.7.5),
        webdav (webdav4 = 0.9.0),
        webdavs (webdav4 = 0.9.0)
Cache types: reflink, hardlink, symlink
Cache directory: apfs on /dev/disk1s1s1
Caches: local
Remotes: https
Workspace directory: apfs on /dev/disk1s1s1
Repo: dvc, git

[skshetry]

It does not support that because you are using dict, only literals are supported. This is because we don't want to execute code, so we use ast.literal_eval internally.

>>> import ast
>>> ast.literal_eval("dict(a=1)")
ValueError: malformed node or string on line 1: <ast.Call object at 0x10a2579d0>

This is working as designed.

[daavoo]

It does not support that because you are using dict, only literals are supported. This is because we don't want to execute code, so we use ast.literal_eval internally.

>>> import ast
>>> ast.literal_eval("dict(a=1)")
ValueError: malformed node or string on line 1: <ast.Call object at 0x10a2579d0>

This is working as designed.

I understand that we don't want to execute arbitrary code, but seems like type constructors could be an optional exception for parsing expressions? (I also recall a discord thread about supporting f-strings, although that might be trickier)

As a side note, the current python params file support feels a little featureless without expressions (https://iterativeai.slack.com/archives/CB41NAL8H/p1629118759096000?thread_ts=1628996368.090800&cid=CB41NAL8H). This might be also addressed via (#6506) where user would take care of whether to evaluate expressions or not.

[daavoo]

Closing in favor of continuing the discussion on #6506 (comment)

[daavoo]

It does not support that because you are using dict, only literals are supported. This is because we don't want to execute code, so we use ast.literal_eval internally.

>>> import ast
>>> ast.literal_eval("dict(a=1)")
ValueError: malformed node or string on line 1: <ast.Call object at 0x10a2579d0>

This is working as designed.

Coming from #6953 , It looks like we are not really using ast.literal_eval

[skshetry]

I was probably confused because I added a fallback to ast.literal_eval as you did in #6954 in https://github.com/iterative/dvc/pull/5778/files#diff-211ccb0a18f425ffc07246dcf8aadc58eb83b04ad4d9eb71f6b70e19654773adL180-R210, but was not able to complete it. Anyway, we should try to have the same behavior as ast.literal_eval as much as possible.