Skip to content
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

Compilation message with MPI extension #5

Merged
merged 12 commits into from
Jan 31, 2024
Merged

Compilation message with MPI extension #5

merged 12 commits into from
Jan 31, 2024

Conversation

ranocha
Copy link
Member

@ranocha ranocha commented Jan 29, 2024

I wanted to draft a version with a package extension for MPI so that we can still include the info message

[ Info: You just called `trixi_include`. Julia may now compile the code, please be patient.

However, the first naive approach doesn't work nicely since we're not allowed to overwrite method definitions while precompiling. Thus, I needed to deactivate precompilation for the extension, resulting in the ugly messages

julia> using MPI, TrixiBase
[ Info: Precompiling TrixiBaseMPIExt [722ac391-ce67-58c0-be25-224aa07e4d5d]
[ Info: Skipping precompilation since __precompile__(false). Importing TrixiBaseMPIExt [722ac391-ce67-58c0-be25-224aa07e4d5d].

julia> trixi_include("test/dummy.jl")
[ Info: You just called `trixi_include`. Julia may now compile the code, please be patient.
Hello

We can not use something in __init__ since that will be called before Trixi.jl is initialized - i.e., before MPI is initialized in general.

test/dummy.jl Outdated Show resolved Hide resolved
Copy link
Member

@sloede sloede left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this work (untried) and is not too dirrrty?

src/trixi_include.jl Outdated Show resolved Hide resolved
src/trixi_include.jl Outdated Show resolved Hide resolved
ext/TrixiBaseMPIExt.jl Outdated Show resolved Hide resolved
@ranocha ranocha requested a review from sloede January 29, 2024 15:22
@ranocha ranocha marked this pull request as ready for review January 29, 2024 15:22
Copy link

codecov bot commented Jan 29, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (1dc5a93) 93.65% compared to head (8bebfd9) 94.36%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main       #5      +/-   ##
==========================================
+ Coverage   93.65%   94.36%   +0.71%     
==========================================
  Files           4        4              
  Lines          63       71       +8     
==========================================
+ Hits           59       67       +8     
  Misses          4        4              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ranocha ranocha changed the title draft MPI extension Compilation message with MPI extension Jan 29, 2024
ext/TrixiBaseMPIExt.jl Show resolved Hide resolved
Copy link
Member

@sloede sloede left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks! If @efaulhaber approves, this can be merged imho (don't mind the missing format check, that's dependent on another PR be to merged)

@ranocha ranocha requested review from sloede and efaulhaber and removed request for sloede January 29, 2024 15:32
sloede
sloede previously approved these changes Jan 29, 2024
test/test_util.jl Outdated Show resolved Hide resolved
it is not empty and ignores some common info statements printed in Trixi.jl
uses.
"""
macro test_nowarn_mod(expr, additional_ignore_content=String[])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we somehow export this as well when importing TrixiBase from a testing environment? Then we can avoid duplicating this function from Trixi.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@efaulhaber Maybe we can move this convo to a separate issue such that we can merge this PR (once you approve) and then start the registration process?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We would need to move this macro to the plain source code of the package and make TrixiBase.jl depend on Test.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that the default

ignore_content = Any["[ Info: You just called `trixi_include`. Julia may now compile the code, please be patient.\n"]

set below will likely also depend on the specific package we're using. For example, we have more stuff in Trixi.jl.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So there is no nice way to do it? Maybe add Test as a soft dependency or so?

Copy link
Member Author

@ranocha ranocha Jan 31, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As @sloede said, I'm fine discussing this further in an issue and merging this soon to start the registration process.

Copy link
Member Author

@ranocha ranocha Jan 31, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tend to prefer moving this to a TrixiTest.jl package to avoid depending on Test etc. everywhere

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved to #9. Your points are absolutely valid @efaulhaber, I just wanted to make sure we can move forward with the current state.

@sloede
Copy link
Member

sloede commented Jan 30, 2024

@ranocha I think the error message is not yet properly filtered out:
https://github.com/trixi-framework/TrixiBase.jl/actions/runs/7711136543/job/21015911732?pr=5#step:7:65

Copy link

github-actions bot commented Jan 31, 2024

Pull Request Test Coverage Report for Build 7726527485

  • 7 of 7 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.7%) to 94.366%

Totals Coverage Status
Change from base Build 7711120067: 0.7%
Covered Lines: 67
Relevant Lines: 71

💛 - Coveralls

@ranocha ranocha requested a review from sloede January 31, 2024 13:07
@ranocha
Copy link
Member Author

ranocha commented Jan 31, 2024

@ranocha I think the error message is not yet properly filtered out: https://github.com/trixi-framework/TrixiBase.jl/actions/runs/7711136543/job/21015911732?pr=5#step:7:65

Should be fixed

@sloede sloede enabled auto-merge (squash) January 31, 2024 13:12
@ranocha ranocha disabled auto-merge January 31, 2024 13:27
@ranocha ranocha merged commit b73ff96 into main Jan 31, 2024
15 checks passed
@ranocha ranocha deleted the hr/mpi_extension branch January 31, 2024 13:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants