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 setting MPI port names of MPI recording and stimulation backends from device labels #2585

Closed
wants to merge 4 commits into from

Conversation

marcelkrueger
Copy link
Contributor

This is PR adds the ability to set the port name for the MPI connection additionally via the device label.

Falls back to normal file based address providing if label is not set.

Originally created for the cosim framework by @mfahdaz

@jougs jougs changed the title Add ability to set mpi port name via label for mpi backends Allow setting MPI port names of MPI recording and stimulation backends from device labels Jan 12, 2023
Copy link
Contributor

@jougs jougs left a comment

Choose a reason for hiding this comment

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

Thinking about this again, I have to admit that I rather dislike the way this feature is implemented by hijacking the device label, which was originally meant to provide a descriptive device identifier for the user.

I think the right way of adding this would be a new device-specific backend property (e.g. mpi_address). See this code in the ASCII recording backend for what I mean. The semantics would be that the address from this new property would used, or read from file if the property is empty.

nestkernel/recording_backend_mpi.cpp Outdated Show resolved Hide resolved
nestkernel/recording_backend_mpi.cpp Outdated Show resolved Hide resolved
nestkernel/recording_backend_mpi.cpp Outdated Show resolved Hide resolved
@jougs jougs added T: Enhancement New functionality, model or documentation S: Normal Handle this with default priority I: External API Developers of extensions or other language bindings may need to adapt their code labels Jan 12, 2023
Co-authored-by: Jochen Martin Eppler <[email protected]>
@jougs
Copy link
Contributor

jougs commented Jan 16, 2023

The same changes would have to be applied also to stimulation_backend_mpi.cpp. However, in order to save you from duplicate work, can you please first comment on my more far-reaching proposal?

@marcelkrueger
Copy link
Contributor Author

marcelkrueger commented Jan 16, 2023

Thinking about this again, I have to admit that I rather dislike the way this feature is implemented by hijacking the device label, which was originally meant to provide a descriptive device identifier for the user.

I agree; this would be especially annoying in cases where multiple recording backends are present that, in the worst case, share a mpi address, and they wouldn't be descriptively distinguishable via the property meant for it.
Also, it would give the label a different meaning only for the mpi backend when for other backends, e.g., the ascii backend, there is already a mechanism to describe such data.

The only question for me is whether it should still be falling back to silently accepting a file for the address or handling it always via the device_data. I.e., providing the address via mpi_address or address_filename.

I like it more when things are written verbosely than relying on documentation to understand behavior. Thus, I would argue for explicitly providing the address always via a device data field. This would also allow free users from having to be careful with the node_id to provide an address and also allow them to reuse the same file in cases where the address is the same.

@jougs
Copy link
Contributor

jougs commented Jan 16, 2023

I think it is probably good enough to have an empty mpi_address property as the default with that meaning to take the address from a file.

If you're willing to go the full length with checks for mutual exclusivity of multiple properties and such, I am also not against it, it just seems to be more work and I'm not sure the small user community of this backend justifies that.

@github-actions
Copy link

Pull request automatically marked stale!

@github-actions github-actions bot added the stale Automatic marker for inactivity, please have another look here label Mar 20, 2023
@jougs
Copy link
Contributor

jougs commented Mar 20, 2023

@marcelkrueger: what is the status of this? Thanks!

@github-actions github-actions bot removed the stale Automatic marker for inactivity, please have another look here label Mar 21, 2023
@med-ayssar
Copy link
Contributor

@marcelkrueger: a gentle ping!

@marcelkrueger
Copy link
Contributor Author

I'm sorry for not getting back to you sooner.
I already swiftly talked to @jougs in a meeting about it but forgot to update here.

Unfortunately, I do not have the capacity right now to implement this properly.
Additionally, it is not high on my priority list as I am not a direct user but just cleaned up the contribution by the original author @mfahdaz.

I might be able to pick it up at a later stage.
However, if it is needed in the meantime anyone can feel free to pick it up.

@github-actions
Copy link

Pull request automatically marked stale!

@github-actions github-actions bot added the stale Automatic marker for inactivity, please have another look here label Jun 19, 2023
@marcelkrueger
Copy link
Contributor Author

I am closing this in favour of the restructured approach in #2858.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I: External API Developers of extensions or other language bindings may need to adapt their code S: Normal Handle this with default priority stale Automatic marker for inactivity, please have another look here T: Enhancement New functionality, model or documentation
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants