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

Allow one to specify an ABI file via MPIPreferences #600

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

sloede
Copy link
Member

@sloede sloede commented May 25, 2022

This is an attempt to fix #574 and to resolve #575. Input welcome.

@simonbyrne
Copy link
Member

should we just make it the abi key?

@vchuravy
Copy link
Member

should we just make it the abi key?

I don't think we can we use that for Platform support.

I am not sure if MPIPreferences is the right place. I would just add it as a preference setting to MPI.jl

@sloede
Copy link
Member Author

sloede commented May 28, 2022

should we just make it the abi key?

I thought about it but decided against it. To me, using this is a last resort when all other options fail. Thus I would not pollute the abi key with this additional functionality. Also, this allows to still set the abi key to something useful (e.g., MichaelMPI. Even though abi_file will ensure that abi is not used for selecting the constants file, it can still be used to document which kind of ABI is in use.

I am not sure if MPIPreferences is the right place. I would just add it as a preference setting to MPI.jl

Really? I thought that MPIPreferences.use_system_binary would be the perfect place, since it controls all other aspects of the implementation selection as well. Further, I think it makes it less intuitive for users if they have to use two different packages (MPIPreferences.jl and MPI.jl) to set preferences.
This is really a design choice, however, thus if you feel strongly about adding this option to MPI.jl directly, I can also add it there (if you tell me where you think it should go).

@vchuravy
Copy link
Member

My reasoning is that the information in MPIPreferences.jl is used by other packages (that's why we separated it), but which mpiconst file to load is really only relevant to MPI.jl

@sloede
Copy link
Member Author

sloede commented May 28, 2022

OK. How would you design the API? Add new functions use_custom_abi_file(path) and use_default_abi_file()? Or set_abi_file!(path_or_nothing)? Or something completely different?

@vchuravy
Copy link
Member

I like use_custom_abi_file and use_default_abi_file for symmetry, but I have no strong feeling.

@sloede
Copy link
Member Author

sloede commented May 28, 2022

There are two potential downsides (that I know of) with putting the preference in MPI.jl and not MPIPreferences.jl

  • It requires an additional section of preferences in LocalPreferences.toml, making it (potentially) harder to see with one glance all relevant MPI preferences.
  • If the abi_file is moved/nonexistent, there is no way to fix the path anymore except by hand, since MPI.jl will fail during precompilation if the file does not exist.

The first one is maybe not so bad, but the second one needs to be handled imho. The only option I see is that one could check for the ABI files existence and it that fails, output something like

@error "Path to ABI file '$abi_file' not found. Fix with `use_custom_abi_file(path)` or `use_default_abi_file()` and restart Julia."

without attempting to include it.

Any thoughts/suggestions?

elseif MPIPreferences.abi == "HPE MPT"
include("mpt.jl")
# If `abi_file` is empty, choose ABI file based on ABI string, otherwise load the specified file
@static if MPIPreferences.abi_file == ""
Copy link
Member

Choose a reason for hiding this comment

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

I think i would prefer if this branch was only taken if abi == "custom" or something similar

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought of the abi_file preference as a "one to rule them all" property that just overrides whatever is set as ABI when it comes to loading the ABI file.

I do see the merits of your suggestion, though. However, what if users set abi_file to something and leave abi unset? Or set it to something other than custom? Wouldn't it be confusing if the abi_file is then ignored? Or should this be handled as an error in the call to use_system_binary?

Copy link
Member

@simonbyrne simonbyrne May 29, 2022

Choose a reason for hiding this comment

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

Given that this option will be rarely used (and is untested), I would be extremely reluctant to make it the "one to rule them all".

The configuration is complicated in any case (which is part of the reason I am reluctant to add this functionality), but I would prefer that any requirements be very explicit. The logic in use_system_binary should handle these cases.

Comment on lines 150 to 154
if isnothing(abi_file)
abi_file = ""
else
abi_file = abspath(abi_file)
end
Copy link
Member

Choose a reason for hiding this comment

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

i'd leave it as nothing if it is not set.

maybe a slightly longer name (e.g. abi_source_file?)

also, you need to store it via set_preferences!

Copy link
Member Author

Choose a reason for hiding this comment

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

i'd leave it as nothing if it is not set.

That won't work, since Nothing is not a valid TOML type:

julia> TOML.print(Dict{String,Any}("abi_file" => nothing))
ERROR: type `Nothing` is not a valid TOML type, pass a conversion function to `TOML.print

This is also the reason why used the empty string as a sentinel. I could also use false if you'd prefer that.

maybe a slightly longer name (e.g. abi_source_file?)

Makes sense, I will do that.

also, you need to store it via set_preferences!

Thanks, I seem to have forgotten to commit that part 🙈

Copy link
Member Author

Choose a reason for hiding this comment

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

maybe a slightly longer name (e.g. abi_source_file?)

Done in 162653a.

Copy link
Member

Choose a reason for hiding this comment

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

That won't work, since Nothing is not a valid TOML type:

Preferences.jl supports it, and it is what is returned when value is not set:

julia> isnothing(load_preference(MPIPreferences, "abi_file"))
true

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

How to create your own consts.jl file manually Restore ability to use MPI implementations with unknown ABIs
4 participants