feat[next-dace]: Less Verbose Warnings#2544
feat[next-dace]: Less Verbose Warnings#2544philip-paul-mueller wants to merge 13 commits intoGridTools:mainfrom
Conversation
|
An alternative would we to simply disable all warnings, but I am not sure if this is a good idea? It is possible to filter out warnings, however, the documentation warns that this feature should not be used in a multi threaded way, which we do. |
| if unit_strides_kind != gtx_common.DimensionKind.HORIZONTAL: | ||
| warnings.warn( | ||
| # TODO(reviewer): I am not sure if we should always print it. | ||
| gtx_transformations.utils.warn( |
There was a problem hiding this comment.
If this is really unexpected, we could also throw an exception.
There was a problem hiding this comment.
It is more a super serious performance warning but (should) not affect correctness.
I think I will not handle it in a special way.
src/gt4py/next/program_processors/runners/dace/transformations/remove_access_node_copies.py
Outdated
Show resolved
Hide resolved
src/gt4py/next/program_processors/runners/dace/transformations/remove_access_node_copies.py
Outdated
Show resolved
Hide resolved
src/gt4py/next/program_processors/runners/dace/transformations/remove_access_node_copies.py
Outdated
Show resolved
Hide resolved
...tests/program_processor_tests/runners_tests/dace_tests/transformation_tests/test_warnings.py
Show resolved
Hide resolved
src/gt4py/next/program_processors/runners/dace/transformations/utils.py
Outdated
Show resolved
Hide resolved
Apperently it want's to run a DaCe test.
The experiment was successfull. This reverts commit 751c226.
There was a problem hiding this comment.
I think the PR can be simplified. Also, IIUC, this PR will only silence our own gt4py warnings, but not DaCe warnings, right? Shouldn't we also silence DaCe warnings with a warning filter or DaCe does not create warnings?
P.S. As warnings filter for DaCe I mean something like:
import sys
if not sys.warnoptions:
import warning
warnings.filterwarnings(
action="ignore",
module="^dace(\..*)?" # Regex pattern matching 'dace' or any submodule
)| @functools.wraps(warnings.warn) | ||
| def warn( | ||
| message: str, | ||
| category: type[Warning] | None = None, | ||
| stacklevel: int = 1, | ||
| source: Any | None = None, | ||
| ) -> None: | ||
| """Wrapper around `warnings.warn()` function that is only enabled in debug mode.""" | ||
| if __debug__: | ||
| # NOTE: The `skip_file_prefixes` argument was introduced in Python 3.12 and is | ||
| # ignored. | ||
| warnings.warn( | ||
| message=message, | ||
| category=category, | ||
| stacklevel=(stacklevel + 1), | ||
| source=source, | ||
| ) |
There was a problem hiding this comment.
The check for __debug__ should be done only once at import time. I also think this can be simplified much further (and also maybe renamed to avoid misunderstandings, although it is not critical):
| @functools.wraps(warnings.warn) | |
| def warn( | |
| message: str, | |
| category: type[Warning] | None = None, | |
| stacklevel: int = 1, | |
| source: Any | None = None, | |
| ) -> None: | |
| """Wrapper around `warnings.warn()` function that is only enabled in debug mode.""" | |
| if __debug__: | |
| # NOTE: The `skip_file_prefixes` argument was introduced in Python 3.12 and is | |
| # ignored. | |
| warnings.warn( | |
| message=message, | |
| category=category, | |
| stacklevel=(stacklevel + 1), | |
| source=source, | |
| ) | |
| if __debug__: | |
| from warnings import warn as debug_warn | |
| else: | |
| @functools.wraps(warnings.warn) | |
| def debug_warn(*args, **kwargs): -> None: | |
| pass |
Finally, I think this is not the right place for this definition. Consider moving it to something like gt4py.next.utils.
There was a problem hiding this comment.
Actually, I've just realized that this redefinition might not be even needed. We could just use the standard warnings.warn function and install a filter for gt4py-produced warnings if __debug__ is false, in the same way I proposed for DaCe.
There was a problem hiding this comment.
The problem is that we have multiple workers and as I understand the documentation doing this is only safe for Python >=3.14, otherwise we would have a data race.
Thus for Pythoon older than 3.14 we would need to inject the filter before we start the workers and maintain the filter (assuming that we use a context) until we are done.
We would thus not only silence the DaCe warnings but pretty much everything.
There was a problem hiding this comment.
The way I understand the documentation is that using context handlers is not thread safe, but I'm proposing to add a global filter at import time for warnings coming from gt4py and dace modules. I don't see why this would be a problem.
For more explicit user control, we could also add a config option/env var GT4PY_SKIP_WARNINGS=0/1 which can be explicitly enabled by the user and otherwise default to filter warnings if not in debug mode. Something conceptually like: skip_warnings = bool(os.env.get(GT4PY_SKIP_WARNINGS, not __debug__))
There was a problem hiding this comment.
That is possible I agree, you just have to make sure that this filter is installed before you start the threads and that you never get rid of the filter.
Do you have an idea where, i.e. which file, we should install this filter?
Directly inside the config.py or is there a better location?
There was a problem hiding this comment.
I'm fine adding this to config.py as a quick workaround for now. It can be cleaned up later in the PR with the new config system.
Some good suggestions from Enrique need to be addressed.
|
|
||
|
|
||
| def test_if_warning_is_raised(): | ||
| assert not gtx_config.SKIP_WARNINGS, "Tests do not run in debug mode." |
There was a problem hiding this comment.
Shouldn't the condition be a more specific?
| assert not gtx_config.SKIP_WARNINGS, "Tests do not run in debug mode." | |
| assert not gtx_config.SKIP_WARNINGS or not __debug__ , "Tests do not run in debug mode." |
| # 3.14, we have to do it here. | ||
| warnings.filterwarnings(action="ignore", module="^dace(\..+)?") | ||
| warnings.filterwarnings( | ||
| action="ignore", module="^gt4py.next.program_processors.runners.dace(\..+)?" |
There was a problem hiding this comment.
| action="ignore", module="^gt4py.next.program_processors.runners.dace(\..+)?" | |
| action="ignore", module="^gt4py.next.program_processors.runners.dace.transformations(\..+)?" |
This PR wraps all warnings such that they are only shown if we are not in debug mode, i.e.
__debug__isTrue.However, it only targets the warnings in the transformations and not the one in DaCe.