Add extensions feature from Julia 1.9 with backward compatibility #98

b-fg · 2024-01-10T21:27:34Z

This allows the conditional loading of the CUDA, AMDGPU, and WriteVTK packages. Code directly depending on these packages is now located in ext/. For example, now the L₂ for CuArray is only loaded when using CUDA is called in addition to using WaterLily.

This is currently a first implementation without propers checks. To do:

Test actual backward compatibility: Tested on 1.8.5 and 1.9.4
Figure out why having the optional packages in the main Julia installation (ie. release) still messes up the initialization
Check CI tests compat and implement do-nothing @allowscalar version so that the CUDA extension can actually extend it: now using GPUArrays.@allowscalar which works for both CUDA and AMD.

This allows the conditional loading of the CUDA, AMDGPU, and WriteVTK packages. Code directly depending on these packages is now located in ext/. This is currently a first implementation without propers checks. ToDo: - Test actual backward compatibility - Figure out why having the optional packages in the main Julia installation (ie. release) still messes up the initialization

…orced in Julia <1.9 versions.

…t.jl). All working as intended now.

b-fg · 2024-01-11T10:26:57Z

This is looking better and close to finishing. Some comments:

CI now only triggers when a new (normal, not draft) PR is created and when a push is done into master, such as a merge from a PR. For new draft PRs, CI is not triggered. Within a PR (normal or draft) new pushes do not trigger the CI - triggering can always be done manually if necessary. This should reduce notifications spam of CI failing and reduce the number of tests that we actually launch (important when migrating to a GPU-enabled CI system).
The new extensions packaging can make the developer workflow a little bit different. Before this, I used to just to develop within the WaterLily.jl environment (cd WaterLily.jl; julia --project; include(...)) and test new features there. Now, if new features make use of a weak dependency such as CUDA or WriteVTK, testing code within the WaterLily.jl environment is problematic since we cannot do using CUDA in there, as the package manager detects that CUDA is no within [deps] (it's in [weakdeps]) and asks to install such package. Instead, I create a new project and environment called WaterLilyTests.jl, I add WaterLily, mark it as development package using Pkg; Pkg.develop(PackageSpec(path="<WaterLily.jl path>"));, and also add whatever packages I need to test development such as CUDA: add CUDA. When WaterLily detects that CUDA is loaded (using CUDA) it will compile the code in ext/WaterLilyCUDAExt.jl and make the extensions available.

…ot pass the WaterLily.jl test section.

b-fg · 2024-01-12T22:28:56Z

So everything is pretty much done. I am locally running the test suite and the only current issue is that while tests are correctly passing for 1.8.5 and 1.9.4, for 1.10.0 are not. Specifically, this test is failing on 1.10.0:
https://github.com/weymouth/WaterLily.jl/blob/fe9827338df7c6e53bfefb749fe32d5a0cf93e74/test/runtests.jl#L298

WaterLily.jl: Test Failed at /home/b-fg/Workspace/tudelft1/WaterLily.jl/test/runtests.jl:298
  Expression: sim_time(sim) ≥ 0.1 > (sum(sim.flow.Δt[1:end - 2]) * sim.U) / sim.L
   Evaluated: 0.07563669234514236 ≥ 0.1 > 0.03125

Any idea why this might be happening @weymouth?

b-fg · 2024-01-12T22:58:47Z

Huh, I just did a merge from master and now is throwing some NaNs. Adding the verbose flag in the test I see:

tU/L=0.0312, Δt=0.355
tU/L=0.0756, Δt=NaN
tU/L=NaN, Δt=NaN

All other test sections are passing in 1.10.0 though.

b-fg · 2024-01-12T23:03:54Z

Seems like exitBC=true is needed in that test so that the simulation does not blow up in 1.10.0. But then again the other tests after this one which use exitBC=false for Array and CuArray work fine... So for the moment I will modify this test and maybe we could compat to 1.9 only until this is figured out.

…hat they pass in Julia 1.10.0.

codecov · 2024-01-12T23:14:56Z

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (b267c6b) 90.42% compared to head (2453e76) 90.55%.

Files	Patch %	Lines
ext/WaterLilyAMDGPUExt.jl	50.00%	1 Missing ⚠️
ext/WaterLilyCUDAExt.jl	50.00%	1 Missing ⚠️
ext/WaterLilyWriteVTKExt.jl	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master      #98      +/-   ##
==========================================
+ Coverage   90.42%   90.55%   +0.13%     
==========================================
  Files           9       11       +2     
  Lines         428      434       +6     
==========================================
+ Hits          387      393       +6     
  Misses         41       41

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

weymouth · 2024-01-13T09:57:00Z

That's weird. Also notice that it's failing on macOS again, even for 1.9. We must have something not quite right in that test.

b-fg · 2024-01-13T09:59:25Z

Yeap!

weymouth · 2024-01-13T12:14:58Z

I'll try it on my home laptop, which is a mac.

b-fg · 2024-01-19T10:40:36Z

Did you have a chance to look at this @weymouth?

weymouth · 2024-01-20T13:11:23Z

Nope, but I'll check this weekend.

Apologies if I've misunderstood, but I don't think we need plotting routines in the test dependancies list, do we?

weymouth · 2024-01-21T14:50:45Z

When I run ] test on this PR, the CUDA and AMDGPU both throw errors when they try to load (since this laptop doesn't have a GPU) but the tests still run and pass. Is that the correct behaviour?

b-fg · 2024-01-21T16:14:05Z

Yes, that is expected since we have it hardcoded in tests hence it tries to load those. Otherwise it would never try to pass them for GPUs, even if CUDA or AMDGPU was functional. I think there is no better way to this at the moment. The main benefit is that for a normal usage (outside of tests) users now do not download and install CUDA/AMDGPU by default.

Project.toml

…ns modules.

b-fg · 2024-01-21T16:35:49Z

ext/WaterLilyAMDGPUExt.jl

+
+L₂ norm of ROCArray `a` excluding ghosts.
+"""
+L₂(a::ROCArray,R::CartesianIndices=inside(a)) = mapreduce(abs2,+,@inbounds(a[R]))


GPU tests cannot really be covered in the current Github CI.

That's clear.

…w). GPU extensions already annotate the L2 function arguments type.

weymouth · 2024-01-21T19:37:45Z

This ready?

b-fg · 2024-01-21T20:21:34Z

Yes, did a last check and all looks good. Also I just passed tests locally on 1.8.5 and 1.10.0 and worked fine. Good to go!

Update to 1.0.4

b-fg added the enhancement New feature or request label Jan 10, 2024

b-fg added 5 commits January 10, 2024 22:29

Bumped CI Julia version to 1.9

1f4f36b

Added extension packages to [extras] so that compatibility can be enf…

be1fa1b

…orced in Julia <1.9 versions.

CI not triggered on github draft PRs.

2440431

CI only automatically applied on push to master or new (non-draft) PRs.

fc6e7cd

Fixed bug in WaterLilyWriteVTKext.jl (previously WaterLilyWriterVTKEx…

a6c5f23

…t.jl). All working as intended now.

CI bug fix.

f2df22c

b-fg mentioned this pull request Jan 11, 2024

Remove useless dependencies from Project.toml #100

Merged

All test suit correctly working for 1.8 and 1.9. However, 1.10 does n…

fe98273

…ot pass the WaterLily.jl test section.

Merge remote-tracking branch 'origin/master' into extensions_1.9

d111b15

b-fg added 2 commits January 13, 2024 00:06

Added exitBC=true to sim_time, and sim_step! stopping time tests so t…

707c9ba

…hat they pass in Julia 1.10.0.

CI running agains for push actions in all branches.

a665a81

b-fg marked this pull request as ready for review January 12, 2024 23:13

Fixed AMDGPU compat import.

89b3eb5

This was referenced Jan 20, 2024

Fix n-bodies stack-overflow error with new type Bodies #95

Merged

Test fail on MacOS in testing enviroment #104

Closed

weymouth added 2 commits January 21, 2024 15:23

Merge branch 'master' into extensions_1.9

0e4a681

Update Project.toml

c375936

Apologies if I've misunderstood, but I don't think we need plotting routines in the test dependancies list, do we?

b-fg commented Jan 21, 2024

View reviewed changes

Project.toml Show resolved Hide resolved

Reverted test fix after hotfix in master and updated docs in extensio…

7b58f8f

…ns modules.

b-fg commented Jan 21, 2024

View reviewed changes

Improved L2 multiple-dispatch for simple Array type (not annotated no…

2453e76

…w). GPU extensions already annotate the L2 function arguments type.

b-fg merged commit bcb925c into master Jan 21, 2024
13 of 14 checks passed

marinlauber pushed a commit to marinlauber/WaterLily that referenced this pull request Jan 22, 2024

merge into a VTKExt from pr-WaterLily-jl#98 and tidy

bf0d589

weymouth mentioned this pull request Mar 29, 2024

Update to 1.0.4 #119

Merged

weymouth referenced this pull request Mar 29, 2024

Merge pull request #119 from weymouth/weymouth-patch-1

c985e9b

Update to 1.0.4

weymouth referenced this pull request Mar 29, 2024

Add compat for EllipsisNotation

b81d536

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add extensions feature from Julia 1.9 with backward compatibility #98

Add extensions feature from Julia 1.9 with backward compatibility #98

b-fg commented Jan 10, 2024 •

edited

Loading

b-fg commented Jan 11, 2024

b-fg commented Jan 12, 2024 •

edited

Loading

b-fg commented Jan 12, 2024 •

edited

Loading

b-fg commented Jan 12, 2024 •

edited

Loading

codecov bot commented Jan 12, 2024 •

edited

Loading

weymouth commented Jan 13, 2024

b-fg commented Jan 13, 2024

weymouth commented Jan 13, 2024

b-fg commented Jan 19, 2024

weymouth commented Jan 20, 2024

weymouth commented Jan 21, 2024

b-fg commented Jan 21, 2024

b-fg Jan 21, 2024

weymouth Jan 21, 2024

weymouth commented Jan 21, 2024

b-fg commented Jan 21, 2024

Add extensions feature from Julia 1.9 with backward compatibility #98

Add extensions feature from Julia 1.9 with backward compatibility #98

Conversation

b-fg commented Jan 10, 2024 • edited Loading

b-fg commented Jan 11, 2024

b-fg commented Jan 12, 2024 • edited Loading

b-fg commented Jan 12, 2024 • edited Loading

b-fg commented Jan 12, 2024 • edited Loading

codecov bot commented Jan 12, 2024 • edited Loading

Codecov Report

weymouth commented Jan 13, 2024

b-fg commented Jan 13, 2024

weymouth commented Jan 13, 2024

b-fg commented Jan 19, 2024

weymouth commented Jan 20, 2024

weymouth commented Jan 21, 2024

b-fg commented Jan 21, 2024

b-fg Jan 21, 2024

Choose a reason for hiding this comment

weymouth Jan 21, 2024

Choose a reason for hiding this comment

weymouth commented Jan 21, 2024

b-fg commented Jan 21, 2024

b-fg commented Jan 10, 2024 •

edited

Loading

b-fg commented Jan 12, 2024 •

edited

Loading

b-fg commented Jan 12, 2024 •

edited

Loading

b-fg commented Jan 12, 2024 •

edited

Loading

codecov bot commented Jan 12, 2024 •

edited

Loading