Support external imports in functions#271
Support external imports in functions#271egparedes merged 7 commits intoGridTools:masterfrom jdahm:support-imports-functions
Conversation
egparedes
left a comment
egparedes
left a comment
There was a problem hiding this comment.
Thanks for finding and fixing this bug. The PR looks fine but there is a typo in the code that should make the test fail. Please fix the typo and verify that the added tests is well defined and covers all the corner cases.
Co-authored-by: Enrique G. Paredes <18477+egparedes@users.noreply.github.com>
|
GitHub Actions failed here. There is no issue with formatting. I'll try to push a minor commit so that it rebuilds after ensuring there is full test coverage. |
| def test_function_import(self, id_version): | ||
| module = f"TestInlinedExternals_test_recursive_imports_{id_version}" | ||
|
|
||
| def some_function(): | ||
| from __externals__ import const | ||
|
|
||
| return const | ||
|
|
||
| def definition_func(inout_field: gtscript.Field[float]): | ||
| from __externals__ import some_call | ||
|
|
||
| with computation(PARALLEL), interval(...): | ||
| inout_field = some_call() | ||
|
|
||
| stencil_id, def_ir = compile_definition( | ||
| definition_func, | ||
| "test_recursive_imports", | ||
| module, | ||
| externals={"some_call": some_function, "const": GLOBAL_CONSTANT}, | ||
| ) | ||
| assert set(def_ir.externals.keys()) == { | ||
| "some_call", | ||
| "const", | ||
| } |
There was a problem hiding this comment.
For reasons that still elude me, this test is necessary. I thought this was covered below, but this uncovered the need for the additional annotate call at gtscript_frontend.py:1477.
| "const", | ||
| "other", | ||
| "func_nest1", | ||
| "tests.test_unittest.test_gtscript_frontend.func_nest1.func_nest2", |
There was a problem hiding this comment.
Externals captured implicitly get the fully qualified name.
egparedes
left a comment
egparedes
left a comment
There was a problem hiding this comment.
The code looks correct although it's hard to say if all the corner cases are covered. Probably the cause of all the weird cases is allowing calls to undecorated functions, which I still think feels like the wrong pattern. Anyway, if you think this PR is done, we can merge it.
|
Yes, I believe this PR is ready, but should have a follow-up that cleans up the annotation system. |
2 participants
Description
Support external imports in functions uniformly. These were already supported if the functions were called inline, however if the functions were an external, they were not resolved.
This contains a small fix for the frontend, as well as a terribly pathological test.
Requirements
Before submitting this PR, please make sure:
new features
Additionally, if this PR contains code authored by new contributors:
signed by the employer if needed, provided to ETH Zurich
version of the AUTHORS.rst file included in the PR