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

Removes busy waiting thread in CLIJxPool by using ArrayBlockingQueue #3

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

Conversation

NicoKiaru
Copy link

Hey @haesleinhuepf !

It's a combo PR / issue report. I'm trying to use your gpu pool instead of writing a new implementation. My goal is to use @bnorthan GPU deconvolution and make it compatible with multiple GPUs (clij/clij2-fft#37)

Regarding this PR:

  • it contains a fix for the busy waiting thread (using ArrayBlockingQueue).
  • it contains a new method to 'free' a CLIJx instance that do not clear the images - the use case is the following: you do not want to push the same psf over and over again for each tile that is processed. What I would prefer rather than my implementation (which was written to avoid breaking backward compatibility) is to remove 'clear' from the setCLIJxIdle method but instead deal with this in the accept method of CLIJxFilterOp. Or simply do not clear at all, it's the responsibility of the callerto avoid such issues.

Finally, I couldn't test any of that, since the N5 dependencies do not compile:

\clijx-faclon-heavy\src\main\java\org\janelia\saalfeldlab\i2k2020\util\N5Factory.java:161:52
java: incompatible types: com.amazonaws.services.s3.AmazonS3URI cannot be converted to java.lang.String

Personally I had a lot of issues with N5 dependencies, and I think it's a good idea to avoid depending on them while they are not fully stabilized, even more when the repo does not deal directly with IO.

What's your opinion on these points ?

Puts Util class as an inner class of CLIJxFilterOp
Removes powerpool method in CLIJxPool
Adds self contained DemoDummyFiltering
Moves Lazy class as inner class of DemoDummyFiltering
Removes N5 dependencies and associated demo and classes
pom.xml:
- puts SNAPSHOT suffix in version
- comment scijava app directory since it is dev specific
- removes non working N5 dependencies
- removes unused picocli dependecy
- put bdv-vistools dependency scope as test
Ignores tests since they are highly dependent on the hardware of the developer
Removes uncompiling demo depending on N5
@NicoKiaru
Copy link
Author

Hi @haesleinhuepf ,

I made a second commit which fixes the dependency issues, essentially by removing every aspect of this repo which depends on N5: - IMO it's not a IO package and it should depend as little as possible on IO packages.

I've instead updated the previous demo with a self-contained demo that should work for everybody, provided that a compatible CLIJ GPU is available.

The commit changes a lot of things as far as the demo is concerned, but should remain fully compatible as far as the pool of CLIJ objects is concerned.

I'm very excited about releasing multi-GPU deconv in Fiji and this repo is a blocking point. Please when you have time let me know if this PR has a chance to be accepted in a not too distant future. Otherwise I could probably go around it by making another CLIJ pool class but that's not optimal.

Cheers,

Nicolas


Here's a recap of the changes:

  • Removes busy waiting thread in CLIJxPool by using ArrayBlockingQueue

Changes package location of CLIJxFilterOp
Puts Util class as an inner class of CLIJxFilterOp
Removes powerpool method in CLIJxPool
Adds self contained DemoDummyFiltering
Moves Lazy class as inner class of DemoDummyFiltering
Removes N5 dependencies and associated demo and classes
pom.xml:

  • puts SNAPSHOT suffix in version
  • comment scijava app directory since it is dev specific
  • removes non working N5 dependencies
  • removes unused picocli dependecy
  • put bdv-vistools dependency scope as test
    Ignores tests since they are highly dependent on the hardware of the developer
    Removes uncompiling demo depending on N5

@NicoKiaru
Copy link
Author

(Note: I also have suggestions regarding the way pools are defined)

@haesleinhuepf
Copy link
Member

Hi @NicoKiaru ,

thanks for working on this! I must admit, I don't fully understand why so many files are deleted in this PR, and I'm not sure if the result remains a good demonstration of distributed / lazy image computing. I have less and less time to work on Java projects btw. Hence I'm curious if you would be interested in taking this repository over and making it yours? Your changes seem very useful!

Thanks!

Best,
Robert

@NicoKiaru
Copy link
Author

Hence I'm curious if you would be interested in taking this repository over and making it yours?

I'd be perfectly happy!

I'm not sure if the result remains a good demonstration of distributed / lazy image computing

There's one, it's using the same dummy filteinr. Just it does not depend on N5 since there are build issues. Also I made it self-contained (it's downloading data and caching from a Zenodo repo). There's no way I could ran the other demo.

@NicoKiaru
Copy link
Author

Thanks for the fast response!

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