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

SharedMemory and NDArray implementation #5

Merged
merged 16 commits into from
Aug 10, 2024
Merged

SharedMemory and NDArray implementation #5

merged 16 commits into from
Aug 10, 2024

Conversation

tpietzsch
Copy link
Contributor

@tpietzsch tpietzsch commented May 16, 2024

This PR adds

1) SharedMemory which is adapted from @carlosuc3m code in JDLL.
The Linux implementation has been tested only superficially, the Windows implementation not at all ...

2) NDArray which annotates SharedMemory with data type and shape, similar to NumPy ndarray.
This is independent of ImgLib2, adapters for ImgLib2 data structures are in https://github.com/imglib/imglib2-appose.

3) JSON de/serialization for SharedMemory and NDArray. Basically: put a NDArray into a Task'sinputs on the Java side and a NDArray comes out in the GroovyWorker inputs, or a NumPy ndarray in the python worker inputs.
(The python side requires the corresponding apposed/appose-python#1 PR)

Copy link
Member

@ctrueden ctrueden left a comment

Choose a reason for hiding this comment

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

Thank you so much @tpietzsch for bringing this to completion. It's great. I am only going to make some small adjustments, which I will list here as part of my review, in case you have any concerns about any of them:

  1. Reformat Java code to use tabs in line with the existing code.
  2. Add class Javadoc to all classes, including proper @author attribution, as well as references to analogous Python API for comparison.
  3. Rewrite the main commit adding the implementation to have a Co-authored-by line crediting @carlosuc3m.
  4. Rename ShmImpl to just Shm, because the Impl suffix in Java classes is traditionally used to indicate an implementation of an interface, not the interface itself.
  5. To avoid the org.apposed.appose base package referencing the org.apposed.appose.shm subpackage: move the ShmImpl/Shm interface out of the Shm subpackage, and alter its create static method to use ServiceLoader instead with platform-specific Shm object factories. Details TBD.

I'll also look into adding a SharedMemoryTest to the unit tests, and broadening the GitHub Actions CI to run on all supported platforms, since this is critical foundational logic.

@ctrueden ctrueden force-pushed the schmarrn branch 3 times, most recently from c1a1194 to 40ce610 Compare July 15, 2024 21:35
tpietzsch and others added 8 commits July 17, 2024 14:38
SharedMemory implementation is lifted from JDLL.
NDArray wraps SharedMemory, and adds shape and data-type information.

Co-authored-by: Carlos Garcia Lopez de Haro <100329787@alumnos.uc3m.es>
(requires branch 'schmarrn' in appose-python to work)
Co-authored-by: Carlos Garcia Lopez de Haro <100329787@alumnos.uc3m.es>
This makes the SharedMemory class into an interface with three
implementations, one per supported platform, but leaves the door open
for additional platforms to add support by implementing their own
ShmFactory that produces appropriate SharedMemory objects.

The plugins are discovered using Java's ServiceLoader mechanism.
There are only two cases: create or attach.
The DType and Shape classes define the two major pieces of metadata
needed by NDArray. One was an inner class of NDArray and the other
was not. This change makes it consistent: now both are inner classes.
@ctrueden ctrueden force-pushed the schmarrn branch 2 times, most recently from 0331a70 to 20fa4af Compare July 17, 2024 20:41
So that it matches the Python implementation of Appose.
The reason it's third in Python is so that it can have the default
parameter value of None, to provide feature parity with the Java
version's two-argument NDAray constructor that creates the SharedMemory.

And create the memory even if the three-argument constructor is used.
Thanks, license-maven-plugin!
@ctrueden ctrueden force-pushed the schmarrn branch 3 times, most recently from 3b4c83c to b228089 Compare July 20, 2024 00:31
And decouple the close and unlink functions.

There was a lot of duplicate code between ShmLinux, ShmMacOS,
and ShmWindows, which is now in the internal ShmBase class.

This made it easier to have consistent logic around the close
vs. unlink behavior across platforms, with less copy/pasted code.
@ctrueden ctrueden merged commit 76c0218 into main Aug 10, 2024
1 check passed
@ctrueden ctrueden deleted the schmarrn branch August 10, 2024 18:26
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.

2 participants