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

Fix img loaders for spark #8

Closed

Conversation

trautmane
Copy link
Contributor

Hi @tpietzsch and @StephanPreibisch,

These commits fix the "could not find XmlIoBasicImgLoader implementation for format bdv.n5" issue @boazmohar identified in JaneliaSciComp/BigStitcher-Spark#2 . The first commit fixes the core problem. The second commit contains slf4j logging calls I added to help confirm what was happening. The logging works well in the spark environment I used for testing, but I wasn't sure if it would cause trouble for other contexts in which spimdata is used. Hopefully, separating the commits makes it easier for you to toss the logging stuff if necessary.

Best,
Eric

@StephanPreibisch
Copy link
Member

Hi @trautmane, that is great, thanks so much!! ... but I don't think it is good to have a log4j dependency in such a basic repo, what do you think @tpietzsch?

@trautmane
Copy link
Contributor Author

Note that the logging dependency is on the slf4j api (not log4j) so it will use whatever logger is bound in by the application using spimdata. If no logger implementation exists, a warning message will be printed upon first reference and then nothing else is logged ( see https://www.slf4j.org/manual.html ).

@tpietzsch
Copy link
Member

Thanks @trautmane!

The registerManually method should be synchronised too probably?
And maybe we should use ConcurrentHashMaps instead of the HashMaps?

@StephanPreibisch I'm not fundamentally opposed to adding logging to the repo, but I also have a slight preference not do it now. Let's keep it on a branch, and next time it proves as useful as here, we merge it in?

@tpietzsch
Copy link
Member

And maybe we should use ConcurrentHashMaps instead of the HashMaps?

I did this, and moved the initial non-synchronized if ( !buildWasCalled ) inside the build() method.
I pushed to the origin/fix_img_loaders_for_spark branch.
@trautmane @StephanPreibisch Could you verify that this still solves your problem and I didn't make any stupid mistake? If it works, I'll merge and release.

I put the slf4j commit on a branch origin/add-logging (which I would not merge for the release).

@trautmane
Copy link
Contributor Author

Hi @tpietzsch - I think your change will still work but I prefer the way I had it (hah - no surprise there :). Maybe I am misunderstanding something. Let's discuss when we meet with @StephanPreibisch at 10am EST today.

@tpietzsch
Copy link
Member

@trautmane I changed it back to how you had it, and merged via origin/fix_img_loaders_for_spark. Releasing now...

@tpietzsch tpietzsch closed this Jan 6, 2022
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.

3 participants