misc: Update advisor with oneAPI 2025 #2533
Conversation
georgebisbas
requested review from
FabioLuporini,
mloubout,
EdCaunt and
JDBetteridge
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
devito/arch/compiler.py
Outdated
| # Locate the Intel Advisor installation and | ||
| # add the necessary flags to the compiler | ||
| from devito.operator.profiling import locate_intel_advisor | ||
| path = locate_intel_advisor() |
There was a problem hiding this comment.
Why aren't all those set by the setvars.sh that's what it's supposed to be for
There was a problem hiding this comment.
Unfortunately, this was true with Intel studios, but now they are not by default. You have to link explicitly. At least, this is what I see.
There was a problem hiding this comment.
And this is actually why it stopped working at some point
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2533 +/- ##
==========================================
- Coverage 87.29% 86.20% -1.10%
==========================================
Files 238 238
Lines 46058 46060 +2
Branches 4080 4079 -1
==========================================
- Hits 40205 39704 -501
- Misses 5163 5661 +498
- Partials 690 695 +5 ☔ View full report in Codecov by Sentry. |
| project = advisor.open_project(str(project)) | ||
|
|
||
| if not project: | ||
| err('Could not open project %s.' % project) | ||
| err(f'Could not open project {project}.') |
There was a problem hiding this comment.
What does open_project return if it fails to open the project? This error message might return something cryptic since project gets overwritten
There was a problem hiding this comment.
I was sure I had replied in this, not sure I cannot see my message. This is part of the advisor API, it should already be there
| @@ -267,86 +272,6 @@ | |||
| "As you can see from this roofline graph, the main point is different from the single point of the previous graph. Moreover, each point is labelled with 'Time' and 'Incidence' indicators. These represent the total execution time of each loop's main body and their percentage incidence on the total execution time of the main time loop." | |||
| ] | |||
| }, | |||
| { | |||
There was a problem hiding this comment.
All this is redundant in my opinion to be treated as-a-worthy to mention API
devito/operator/profiling.py
Outdated
| compiler.add_library_dirs(libdir) | ||
| compiler.add_ldflags('-Wl,-rpath,%s' % libdir) | ||
| super(AdvisorProfiler, self).__init__(name) | ||
| return |
There was a problem hiding this comment.
AFAICT, this is just yet another empty constructor? droppable?
| if self.path is None: | ||
| self.initialized = False | ||
| else: | ||
| super().__init__(name) |
There was a problem hiding this comment.
I prefer to keep this logic here rather than in compiler.py, which shouldn't be aware of profiling
devito/arch/compiler.py
Outdated
| @@ -771,6 +771,18 @@ def __init_finalize__(self, **kwargs): | |||
| self.__init_intel_mpi__() | |||
| self.__init_intel_mpi_flags__() | |||
|
|
|||
| if configuration['profiling'] == 'advisor': | |||
There was a problem hiding this comment.
see comment below, but basically since "profiling" requires third-party tools (as opposed to MPI which is an actual API) I have a preference for this bit of logic to be kept in profiling
georgebisbas
force-pushed
the
advisor_refresh_II
branch
3 times, most recently
from
4c1f058 to
ed910e5
Compare
devito/operator/operator.py
Outdated
| @@ -219,6 +219,12 @@ def _build(cls, expressions, **kwargs): | |||
|
|
|||
| # Required for the jit-compilation | |||
| op._compiler = kwargs['compiler'] | |||
|
|
|||
| # Add any metadata from the profiler | |||
There was a problem hiding this comment.
nitpicking, I wouldn't call it "metadata"
There was a problem hiding this comment.
hope is better
conftest.py
Outdated
| @@ -80,7 +80,7 @@ def skipif(items, whole_module=False): | |||
| skipit = "`icx+cpu64` won't work with this test" | |||
| break | |||
| # Skip if icx or advisor are not available | |||
| if i not in 'only-advisor' or \ | |||
| if i not in ('noadvisor') or \ | |||
There was a problem hiding this comment.
no this is wrong as it's not a tuple
There was a problem hiding this comment.
omg, forgot the comma
devito/operator/profiling.py
Outdated
| libdir = path.joinpath('lib64').as_posix() | ||
| self._lib_dirs = [libdir] | ||
| else: | ||
| warning("Intel Advisor not found; reverting to `advanced`") |
There was a problem hiding this comment.
is it really reverting?
There was a problem hiding this comment.
good catch, indeed it is reverting, but there is another mini-bug. It was reverting to basic, now it is reverting to advanced, and dropped a double-emmited warning due to this
georgebisbas
force-pushed
the
advisor_refresh_II
branch
from
d0ff002 to
b79acd3
Compare
| warning("Couldn't set up `%s` profiler; reverting to `advanced`" % level) | ||
| profiler = profiler_registry['basic'](name) |
There was a problem hiding this comment.
Here, it was reverting to basic
There was a problem hiding this comment.
ok but no need for that extra var imho (nitpicking)
There was a problem hiding this comment.
well this was only for a better fstring...but ok
devito/operator/profiling.py
Outdated
| if path: | ||
| self._include_dirs = [path.joinpath('include').as_posix()] | ||
| libdir = path.joinpath('lib64').as_posix() | ||
| self._lib_dirs = [libdir] |
There was a problem hiding this comment.
no, instances shouldn't change class attributes like that....
and btw why is it even a class attribute ?
There was a problem hiding this comment.
What should it have been?
| warning("Couldn't set up `%s` profiler; reverting to `advanced`" % level) | ||
| profiler = profiler_registry['basic'](name) |
There was a problem hiding this comment.
ok but no need for that extra var imho (nitpicking)
FabioLuporini
changed the title
georgebisbas
force-pushed
the
advisor_refresh_II
branch
from
398b613 to
53a26bd
Compare
devito/operator/profiling.py
Outdated
| @@ -52,8 +50,18 @@ def __init__(self, name): | |||
| # Python-level timers | |||
| self.py_timers = OrderedDict() | |||
|
|
|||
| # Instance attributes | |||
There was a problem hiding this comment.
I'd better change/improve I think this comment
devito/operator/profiling.py
Outdated
| @@ -52,8 +50,18 @@ def __init__(self, name): | |||
| # Python-level timers | |||
| self.py_timers = OrderedDict() | |||
|
|
|||
| # Instance attributes | |||
| self._include_dirs = [] | |||
There was a problem hiding this comment.
since these attributes (and the corresponding _add methods (please make them private)) are used only required by "third party profilers", you should move them into the Advisor class , or inside an intermediate class for ThirdPartyProfilers/ExternalProfilers
because here they'll never ever be used by the standard Profiler classes
There was a problem hiding this comment.
oh, I see why now...
because you need these attributes in operator.py
There was a problem hiding this comment.
So, no changes here AFAIU
georgebisbas
force-pushed
the
advisor_refresh_II
branch
from
da91de6 to
e3c2211
Compare
Some more changes are needed, but finally works