sacred
sacred copied to clipboard
IDSIA
Allow config scopes with type annotations.
Allows the usage of type hints in config scopes. Closes #818.
Notes:
- A new test is added for this behaviour.
- Instead of fixing the regexp, it uses
astandtokenizemodules. Withastwe find the first line with code in the function body. As comments are removed in the AST representation, we then generate the tokens of the function until that line is reached, in order to keep the previous lines that only have comments and whitespace.
Hi @vnmabus! I seem to have overlooked your PR. I like the idea to use the ast module to parse the config scopes, especially because bugs related to the config scope parsing keep popping up here and there, and we currently have to modify the regexes every time things are added to the language. But the current implementation doesn't work. Are you planning to fix the issues?
Hum, it worked in my PC. I can't see the errors in Azure (maybe because the runs have been removed). Can you relaunch the tests?
When I try to run the testcases, I get the error E NameError: name 'generate_tokens' is not defined
GitHub doesn't let me re-run the checks. I clicked on the re-run button and it just disappeared without re-running the checks. The easiest is probably to push an empty commit, which should trigger the checks again.
It seems that it is working only in Python 3.8 and newer because of this: https://docs.python.org/3/whatsnew/3.8.html#tokenize . I could attempt to manually add a newline to have it working in Python 3.7. However, if we are following https://numpy.org/neps/nep-0029-deprecation_policy.html we should remove support for Python 3.7 anyways.
@vnmabus Agreed, if it makes your code significantly simpler, we can remove support for Python 3.7
@vnmabus Agreed, if it makes your code significantly simpler, we can remove support for Python 3.7
I don't think it would be too hard to fix it either. But if you are planning to upgrade the minimum requirements soon I won't need to do it.
I don't plan on upgrading the minimum requirements explicitly in a separate PR. The recent updates always happened as a byproduct of other changes, like this one.
Well, probably it is cleaner to modify the files related with the minimum version (setup.py/pyproject.toml, tests, etc) in a separate PR. What do you think?
I found one issue:
def fn(): a = 2 # valid python code
get_function_body(fn)
gives
('def fn(): a = 2 # valid python code\n', 1)
The old variant threw an exception here. This now results in:
from sacred.config.config_scope import ConfigScope
@ConfigScope
def fn(): a = 2
fn()
{'fn': <function __main__.fn()>}
It should either work as expected or fail with a good error message
It should either work as expected or fail with a good error message
Good catch!! I decided to make it work. I had to change the expected results of two tests, as now the comment after the function signature is also preserved (and the newlines too).