-
Notifications
You must be signed in to change notification settings - Fork 55
feat[next-dace]: Less Verbose Warnings #2544
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
base: main
Are you sure you want to change the base?
Changes from 10 commits
383b5c2
f7734a5
ea1610d
22902dc
bbe84de
4b2292e
cbb18fe
751c226
b1870f6
5f4c2bf
6ed3788
c50576d
092ac76
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -8,8 +8,10 @@ | |||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
| """Common functionality for the transformations/optimization pipeline.""" | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
| import functools | ||||||||||||||||||||||||||||||||||||||||||||||||
| import uuid | ||||||||||||||||||||||||||||||||||||||||||||||||
| from typing import Optional, Sequence, TypeVar, Union | ||||||||||||||||||||||||||||||||||||||||||||||||
| import warnings | ||||||||||||||||||||||||||||||||||||||||||||||||
| from typing import Any, Optional, Sequence, TypeVar, Union | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
| import dace | ||||||||||||||||||||||||||||||||||||||||||||||||
| from dace import data as dace_data, libraries as dace_lib, subsets as dace_sbs, symbolic as dace_sym | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -21,6 +23,25 @@ | |||||||||||||||||||||||||||||||||||||||||||||||
| _PassT = TypeVar("_PassT", bound=dace_ppl.Pass) | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
| @functools.wraps(warnings.warn) | ||||||||||||||||||||||||||||||||||||||||||||||||
| def warn( | ||||||||||||||||||||||||||||||||||||||||||||||||
| message: str, | ||||||||||||||||||||||||||||||||||||||||||||||||
| category: type[Warning] | None = None, | ||||||||||||||||||||||||||||||||||||||||||||||||
| stacklevel: int = 1, | ||||||||||||||||||||||||||||||||||||||||||||||||
| source: Any | None = None, | ||||||||||||||||||||||||||||||||||||||||||||||||
philip-paul-mueller marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||||||||||||||||||||||||||||||||||||||||||||
| ) -> 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, | ||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||
| @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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,26 @@ | ||
| # GT4Py - GridTools Framework | ||
| # | ||
| # Copyright (c) 2014-2024, ETH Zurich | ||
| # All rights reserved. | ||
| # | ||
| # Please, refer to the LICENSE file in the root directory. | ||
| # SPDX-License-Identifier: BSD-3-Clause | ||
|
|
||
| import pytest | ||
| import numpy as np | ||
| import copy | ||
|
|
||
| # Without this the test fails if DaCe is not installed, even when the `requires_dace` | ||
| # marker is configured in `__init__.py`. | ||
| dace = pytest.importorskip("dace") | ||
|
|
||
| from gt4py.next.program_processors.runners.dace import ( | ||
| transformations as gtx_transformations, | ||
| ) | ||
|
|
||
|
|
||
| def test_if_warning_is_raised(): | ||
| warn_msg = "This is a warning." | ||
|
|
||
| with pytest.warns(UserWarning, match=warn_msg): | ||
philip-paul-mueller marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| gtx_transformations.utils.warn(warn_msg, UserWarning) | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is really unexpected, we could also throw an exception.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is more a super serious performance warning but (should) not affect correctness.
I think I will not handle it in a special way.