-
Notifications
You must be signed in to change notification settings - Fork 13
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
Create an internal API which deploys off of JACCPreferences.backend
#86
Open
kmp5VT
wants to merge
24
commits into
JuliaORNL:main
Choose a base branch
from
kmp5VT:kmp5/refactor/internals
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 4 commits
Commits
Show all changes
24 commits
Select commit
Hold shift + click to select a range
785d0af
Modify all CPU code
kmp5VT ca0d9de
More updates, GPU and array portion
kmp5VT 50bd061
convert Array to array
kmp5VT 694c2c8
Add comment
kmp5VT a8f9158
Allow JACC to precompile convert preference to symbol and use that to…
kmp5VT c1ffbbc
explicitly overwrite JACC.arraytype
kmp5VT a31b1d9
Add error about backend package not loaded.
kmp5VT 17522a8
remove comment
kmp5VT 70410a9
update AMDGPU tests
kmp5VT 75a2040
update oneapi tests
kmp5VT 5180159
Merge remote-tracking branch 'origin/main' into kmp5/refactor/internals
kmp5VT 89b2301
Don't precompile
kmp5VT 599aa7d
Move JACC_BACKEND_TYPE to JACCPreferences
kmp5VT fea569a
Merge branch 'main' into kmp5/refactor/internals
kmp5VT bb4fb35
Move JACCArrayType to its own file and bring into preferences.jl
kmp5VT 5632023
Fix JACC call in JACC.BLAS
kmp5VT be2cec1
Small updates to threads test
kmp5VT 04c9b65
Merge branch 'kmp5/refactor/internals' of https://github.com/kmp5VT/J…
kmp5VT d497ea8
Make sure eltypes are consistent in cuda tests
kmp5VT 2f95204
Add back JACC BLAS, works fine on AMDGPU
kmp5VT 28380a0
Merge branch 'main' into kmp5/refactor/internals
kmp5VT 60e6387
Small updates to experimental `shared` function
kmp5VT 09f6a33
`test_threads.jl` now can be run with any JACC backend
kmp5VT 00f2ca8
Updates to tests since test_threads supports different backends
kmp5VT File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file was deleted.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file was deleted.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,8 +1,52 @@ | ||
|
||
function zeros(T, dims...) | ||
return Base.zeros(T, dims...) | ||
return fill!(similar(arraytype(){T}, dims...), zero(T)) | ||
end | ||
|
||
function ones(T, dims...) | ||
return Base.ones(T, dims...) | ||
return fill!(similar(arraytype(){T}, dims...), one(T)) | ||
end | ||
|
||
array(T::AbstractArray) = arraytype()(T) | ||
|
||
function parallel_for(::JACCArrayType{<:Array}, N::Integer, f::Function, x...) | ||
@maybe_threaded for i in 1:N | ||
f(i, x...) | ||
end | ||
end | ||
|
||
function parallel_for(::JACCArrayType{<:Array}, | ||
(M, N)::Tuple{Integer, Integer}, f::Function, x...) | ||
@maybe_threaded for j in 1:N | ||
for i in 1:M | ||
f(i, j, x...) | ||
end | ||
end | ||
end | ||
|
||
function parallel_reduce(::JACCArrayType{<:Array}, N::Integer, f::Function, x...) | ||
tmp = Base.zeros(Threads.nthreads()) | ||
ret = Base.zeros(1) | ||
@maybe_threaded for i in 1:N | ||
tmp[Threads.threadid()] = tmp[Threads.threadid()] .+ f(i, x...) | ||
end | ||
for i in 1:Threads.nthreads() | ||
ret = ret .+ tmp[i] | ||
end | ||
return ret | ||
end | ||
|
||
function parallel_reduce(::JACCArrayType{<:Array}, | ||
(M, N)::Tuple{Integer, Integer}, f::Function, x...) | ||
tmp = Base.zeros(Threads.nthreads()) | ||
ret = Base.zeros(1) | ||
@maybe_threaded for j in 1:N | ||
for i in 1:M | ||
tmp[Threads.threadid()] = tmp[Threads.threadid()] .+ f(i, j, x...) | ||
end | ||
end | ||
for i in 1:Threads.nthreads() | ||
ret = ret .+ tmp[i] | ||
end | ||
return ret | ||
end |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
We are trying to keep the API as simple as possible for domain scientists. Please see #62 for a similar discussion.
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 think I am confused. The outward facing API is exactly the same as the current implementation its just these two functions
Internally JACC will create a struct based on the
JACC.JACCPreference.backend
variable and will only allow users to utilize a single Array backend type just like the current implementation. The only real outward facing change for the current users isJACC.Array -> JACC.array
as to not export and overwriteBase.Array
which could cause issues in other libraries.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.
This PR is breaking the current API. What I am trying to understand is the value added for this approach, we are adding internal boilerplate, but the end result in similar to the current implementation unless I am missing something. As for pre-compilation, we intentionally want to prevent compiling on non-supported platforms as we had a number of reported issues in the past on our systems. Multiple dispatch is a JIT "runtime concept" we are trying to avoid at all with weakdependencies.
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.
See for example my comment here v0.0.3 wouldn't even compile when JACC.jl is consumed from an application and would just hang. Adding more applications help reveal a lot of these type of issues.
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.
We are not overwriting Base.Array , JACC.Array is just an alias to existing Array types. Also, it's not clear what issues this could cause unless JACC.Array is misused outside its current scope. We can always implement a
JACC.ToHost(JACC.Array)::Base.Array
function to guarantee a host array.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.
Can you please provide an example where this implementation breaks the current API outside of migrating
Array
to anarray
function?I have found some issues with the current main branch
If you don't have the correct backend loaded then JACC will still run with "threads" because
JACC.Array = Base.Array
. This would be unexpected behavior. The system I have written here forces the component matchingJACC.JACCPreferences.backend
to be loaded for the user facing API to work.Currently the alias to array makes a global variable called
Array
and exports it which effectively overwrites theBase.Array
. This can easily cause an issue outside of JACCThe system here proposed does not need to alias Array because the alias is effectively stored as a compiled parameter inside the module.
Another issue is that if a different backend introduced after loading JACC, the definition of
JACC.Array
changesThis is not possible with the implementation proposed because the forward facing API effectively chooses the backend based solely on the
JACCPreferences.backend
variable.I understand that you don't want to mix backends and that is specifically why I designed the code to internally construct the
JACCArrayType
based on theSymbol(JACCPreferences.backend)
variable. To enforce this we should only document the two functions inJACC.jl
and suggest using theJACC.JACCPreferences.set_backend
variable to change the backend arraytype.In regards to the JIT and multiple dispatch, I am still learning Julia so I could be wrong but I am not sure that is a concern based on the design here. The function
JACC_BACKEND_TYPE()
is compiler infer- able and defined at compile timeThere 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 think this is self-explanatory.
using X
is certainly not recommended, butimport X
due to name clashing, I think this is different from overriding Base.Array.Packages still need to be downloaded and precompiled. That's where we saw the reported issues on our systems. Hence why we are being very conservative as explained in our last call. Even more with relatively new Julia version, features and rapidly evolving stack for AMD and Intel GPUs.
I just think the motivation and problems tackled for our systems by the current API and implementation are very different from those in this PR.
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.
@williamfgc I do not understand your systems as the code patterns here work on my clusters without issue and all of the unittests in the module pass. I would love to understand how this PR breaks on your systems if you could run the code in this PR and give me examples that would be appreciated. I specifically tailored the design this PR from our conversations on Zoom. Further, I connected with @pedrovalerolara to collaborate with you on code design with the potential of integrating JACC into the ITensors. I do not have a specific use cases for your code and have not used any of your software previously.
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 would be a good starting point for collaboration. Feel free to integrate JACC with ITensors and understand the trade-offs and the motivation of the current design before changing all internals. As discussed in the call, we target DOE HPC systems, which are not simple to deploy and use, especially with new stuff like the Julia ecosystem.
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.
Alternative idea: can we do
struct Array{T,N} end
and then haveArray{T,N}(...) = JACC.arraytype(){T,N}(...)
? This would preserve the idea thatJACC.Array
can become another array type, without messing with modifying module globals at runtime.