Skip to content
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

[Python frontend]: Python 3.11.7 + enumerate + subscript breakage #1568

Closed
FlorianDeconinck opened this issue May 6, 2024 · 2 comments · Fixed by #1570
Closed

[Python frontend]: Python 3.11.7 + enumerate + subscript breakage #1568

FlorianDeconinck opened this issue May 6, 2024 · 2 comments · Fixed by #1570

Comments

@FlorianDeconinck
Copy link
Contributor

FlorianDeconinck commented May 6, 2024

Describe the bug
The interaction of python 3.11.7 and enumerate + subscript is breaking the parsing. The following code is a reduction of a code in NASA/NOAA dynamical core. This works with python 3.8.X

To Reproduce

With python-3.11.7:

import numpy as np
import dace

tracer_variables = ["vapor", "rain", "nope"]


@dace.program
def enumerate_parsing(
    A,
    tracers: dace.compiletime,  # Dict[str, np.float64]
):
    for i, q in enumerate(tracer_variables[0:2]):
        tracers[q][:] = A


sz = (3, 3)
A_field = np.zeros(sz)
all_qs = {
    "ice": np.ones(sz),
    "rain": np.ones(sz),
    "vapor": np.ones(sz),
}

dace.config.Config.set(
    "frontend",
    "unroll_threshold",
    value=False,
)

enumerate_parsing(A_field, all_qs)

Traceback:

...
  File "/home/fgdeconi/work/git/ndsl/external/dace/dace/frontend/python/preprocessing.py", line 764, in visit_Subscript
    return self._visit_potential_constant(node.value.elts[gslice], True)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/fgdeconi/work/git/ndsl/external/dace/dace/frontend/python/preprocessing.py", line 735, in _visit_potential_constant
    return self.generic_visit(node)
           ^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/fgdeconi/work/git/ndsl/external/dace/dace/frontend/python/preprocessing.py", line 486, in generic_visit
    return super().generic_visit(node)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/fgdeconi/work/git/ndsl/external/dace/dace/frontend/python/astutils.py", line 453, in generic_visit
    for field, old_value in ast.iter_fields(node):
  File "/home/fgdeconi/.pyenv/versions/3.11.7/lib/python3.11/ast.py", line 260, in iter_fields
    for field in node._fields:
                 ^^^^^^^^^^^^
AttributeError: 'list' object has no attribute '_fields'
@alexnick83
Copy link
Contributor

First evaluation:

I checked the code snippet above with both Python versions, and the issue at a superficial level is the following. In Python 3.8, the ast.Slice of an ast.Subscript does not contain line and column number information. In Python 3.11, the ast.Slice has all the information. Due to the missing numbers in Python 3.8, parsing follows a different path, which doesn't crash.

Information for further debugging (@tbennun maybe you have an insight on this?):
Place a breakpoint at dace/frontent/python/preprocessing line 750:

            try:
                gslice = astutils.evalnode(node.slice, self.globals)
            except SyntaxError:
                return self.generic_visit(node)

@FlorianDeconinck
Copy link
Contributor Author

I haven't dug in this code so much, but it seem to me something changed to _visit_potential_constant which made the calling code non-relevant anymore for list.

Indeed in 3.8 it all failed so the return is just the same node.

I pushed a PR that, restrict the list to the slice then try to turn elements into constant, then return the corrected list. Works for my use case.

@alexnick83 alexnick83 linked a pull request May 8, 2024 that will close this issue
github-merge-queue bot pushed a commit that referenced this issue May 8, 2024
Looking at: #1568

The code was blindly calling down to a `_visit_potential_constant` which
is written for single element rather collection of them. Unroll the
list, like the `dict` is done in the `if` above.
github-merge-queue bot pushed a commit that referenced this issue May 9, 2024
Follow up of #1460 

- [x] Fixed the `ci` script (including `git checkout issues` around
selecting the correct `dace`)
- [x] Move `D_SW` to execute only on rank 0 to avoid rebuild
- [x] Swapped Rieman Solver on C-grid for D-grid for better coverage

~~WARNING: this PR is blocked by #1477~~
~~WARNING: this PR is blocked by #1568~~

---------

Co-authored-by: Tal Ben-Nun <tbennun@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants