misc: Update advisor with oneAPI 2025 - #2533
Conversation
Sorry, something went wrong.
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
Sorry, something went wrong.
| # 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
Sorry, something went wrong.
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.
Sorry, something went wrong.
There was a problem hiding this comment.
And this is actually why it stopped working at some point
Sorry, something went wrong.
Codecov ReportAttention: Patch coverage is 41.66667% with 28 lines in your changes missing coverage. Please review.
@@ Coverage Diff @@
## master #2533 +/- ##
==========================================
+ Coverage 87.52% 87.55% +0.03%
==========================================
Files 238 238
Lines 47034 47032 -2
Branches 4144 4143 -1
==========================================
+ Hits 41165 41178 +13
+ Misses 5176 5161 -15
Partials 693 693 ☔ View full report in Codecov by Sentry.
|
Sorry, something went wrong.
|
|
||
| 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
Sorry, something went wrong.
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
Sorry, something went wrong.
| "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
Sorry, something went wrong.
| 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.
no need
Sorry, something went wrong.
There was a problem hiding this comment.
AFAICT, this is just yet another empty constructor? droppable?
Sorry, something went wrong.
| 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
Sorry, something went wrong.
| # 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"
Sorry, something went wrong.
There was a problem hiding this comment.
hope is better
Sorry, something went wrong.
| 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
Sorry, something went wrong.
There was a problem hiding this comment.
omg, forgot the comma
Sorry, something went wrong.
| 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?
Sorry, something went wrong.
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
Sorry, something went wrong.
| 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
Sorry, something went wrong.
There was a problem hiding this comment.
ok but no need for that extra var imho (nitpicking)
Sorry, something went wrong.
There was a problem hiding this comment.
well this was only for a better fstring...but ok
Sorry, something went wrong.
| 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 ?
Sorry, something went wrong.
There was a problem hiding this comment.
What should it have been?
Sorry, something went wrong.
| 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)
Sorry, something went wrong.
|
|
||
| path = get_advisor_path() | ||
| if path: | ||
| self.add_include_dir(path.joinpath('include').as_posix()) |
There was a problem hiding this comment.
as explained above, this should just be self._include_dirs.append(...)
Sorry, something went wrong.
There was a problem hiding this comment.
yes
Sorry, something went wrong.
| break | ||
| # Skip if icx or advisor are not available | ||
| if i == 'noadvisor' and \ | ||
| not isinstance(configuration['compiler'], IntelCompiler) or \ |
There was a problem hiding this comment.
this is wrong again, I think, because of and/or operator precedence...
The fact we got this wrong multiple times and nothing breaks on CI suggests it's not very useful maybe
Sorry, something went wrong.
| if path: | ||
| self._include_dirs.append(path.joinpath('include').as_posix()) | ||
| self._lib_dirs.append(path.joinpath('lib64').as_posix()) | ||
| self._attempted_init = True |
There was a problem hiding this comment.
maybe for another day, but these three lines
self._attempted_init = True
else:
self._attempted_init = False
could become a single line
self._attempted_init = bool(path)
but anyway, nitpicking... maybe you can do it in another PR
Sorry, something went wrong.
There was a problem hiding this comment.
but why append then if path is None? If the compiler reverts to advanced in the case of advisor's absence, the paths will be linked right?
Sorry, something went wrong.
| "output_type": "stream", | ||
| "text": [ | ||
| "\u001b[1;37;32mOpening project...\u001b[0m\n", | ||
| "\u001b[1;37;32mOpening project /home/gb4018/workspace/devitocodes/devito/examples/performance/profilings/JupyterProfiling...\u001b[0m\n", |
There was a problem hiding this comment.
This is not OK, I don't like personal dirs in logs (ie gb4018)
Sorry, something went wrong.
| "output_type": "stream", | ||
| "text": [ | ||
| "\u001b[1;37;32mOpening project...\u001b[0m\n", | ||
| "\u001b[1;37;32mOpening project /home/gb4018/workspace/devitocodes/devito/examples/performance/profilings/JupyterProfiling...\u001b[0m\n", |
There was a problem hiding this comment.
same here
Sorry, something went wrong.
| # Required for the jit-compilation | ||
| op._compiler = kwargs['compiler'] | ||
|
|
||
| # Required for compilation by the profiler |
There was a problem hiding this comment.
Why aren't those part of the profller init?
Sorry, something went wrong.
There was a problem hiding this comment.
It was before, and this was the reason it was not working, as a different compiler was build after doing the profiler init. Related discussion here: https://devitocodes.slack.com/archives/C7JMLMSG0/p1739458707616429?thread_ts=1739458681.588539&cid=C7JMLMSG0
Sorry, something went wrong.
JDBetteridge
left a comment
There was a problem hiding this comment.
Nitpicks. But otherwise LGTM
Sorry, something went wrong.
| # saving the chart in PNG format | ||
| plt.savefig('%s.png' % name, bbox_extra_artists=(legend,), bbox_inches='tight') | ||
| # saving the chart in PDF format | ||
| plt.savefig('%s.pdf' % name, bbox_extra_artists=(legend,), bbox_inches='tight') |
There was a problem hiding this comment.
| plt.savefig('%s.pdf' % name, bbox_extra_artists=(legend,), bbox_inches='tight') | |
| plt.savefig(f'{name}.pdf', bbox_extra_artists=(legend,), bbox_inches='tight') |
Sorry, something went wrong.
| generate a roofline. Both are run by `run_advisor.py`. | ||
| * Requires python3, untested in earlier versions of python and conda environments | ||
| * Currently requires download of repository and running `pip3 install .`, the scripts | ||
| * Requires Python3, untested in conda environments |
There was a problem hiding this comment.
| * Requires Python3, untested in conda environments | |
| * Requires Python 3.9 or later, untested in conda environments |
Let's not use unsupported Python versions!
Sorry, something went wrong.
| plt.savefig('%s.pdf' % name, bbox_extra_artists=(legend,), bbox_inches='tight') | ||
| figpath = os.path.realpath(__file__).split(os.path.basename(__file__))[0] | ||
| log('Figure saved in %s%s.png.' % (figpath, name)) | ||
| log('\nFigure saved in %s%s.pdf.' % (figpath, name)) |
There was a problem hiding this comment.
| log('\nFigure saved in %s%s.pdf.' % (figpath, name)) | |
| log(f'\nFigure saved in {figpath}{name}.pdf.') |
Sorry, something went wrong.
| f.write(json.dumps(roofline_data)) | ||
|
|
||
| log('JSON file saved as %s.json.' % name) | ||
| log('\nJSON file saved as %s.json.' % name) |
There was a problem hiding this comment.
| log('\nJSON file saved as %s.json.' % name) | |
| log(f'\nJSON file saved as {name}.json.') |
Sorry, something went wrong.
Co-authored-by: Jack Betteridge <43041811+JDBetteridge@users.noreply.github.com>
|
Merged, thanks |
Sorry, something went wrong.
Some more changes are needed, but finally works