scijava/incubator

Where development happens and stuff breaks 🛠️

Curtis Rueden

@ctrueden

It's how you would do it?

Giuseppe Barbieri

@elect86

you can tune the scan, but are we talking about an Annotation or?

Curtis Rueden

@ctrueden

For now let's say no annotation.

Will the try-with-resources result in a new classpath scan per discover call? Or does classgraph cache things?

Giuseppe Barbieri

@elect86

as far as I recall, the scan is the bottleneck, so the less, the better

ideally, you'd do this just once at the begin and then re-use it whenever you want to extract anything

Curtis Rueden

@ctrueden

Agreed, but I wasn't sure if it was OK to save the ScanResult as a field of the Discoverer. That goes against the classgraph docs which say you should dispose of your ScanResult ASAP.

Giuseppe Barbieri

@elect86

indeed, there seems to be some allocated resources alongside..

Gabriel Selzer

@gselzer

@ctrueden I think this looks great.

ideally, you'd do this just once at the begin and then re-use it whenever you want to extract anything

I think at the end of the day you are going to be caching the results of the discover call, so I can't see a situation where you're going to be calling discover enough times to be worried about this?

Curtis Rueden

@ctrueden

I'm worried about calling discover(Something.class) and then discover(SomethingElse.class) and then discover(AThirdThing.class) causing three full classpath scans.

Gabriel Selzer

@gselzer

We could write some fancy logic to time it out after a couple seconds?

Curtis Rueden

@ctrueden

Yeah, I was thinking something similar... that would be quite evil! Let's do it later, if/when we cross that bridge.

Giuseppe Barbieri

@elect86

how often do you need to retrieve stuff?

Gabriel Selzer

@gselzer

Idk, I think your intuition to follow the documentation and discard right away is probably correct

In the short term

Curtis Rueden

@ctrueden

It's not really about "how often" as much as "how many different types of things"

Giuseppe Barbieri

@elect86

I imagine scijava just loads all the @ScijavaPlugins at the start and that's it

Curtis Rueden

@ctrueden

And I'd say... maybe 20-50 types of things?

For sure, for SciJava 2, loading everything with @Plugin would be straightforward.

Giuseppe Barbieri

@elect86

also, there are some news starting from jdk16+..

Gabriel Selzer

@gselzer

Alright, @ctrueden, I've been thinking more about how the TherapiOpInfoDiscoverer

47 replies

Curtis Rueden

@ctrueden

@gselzer Made some progress on the PR chain:

Merged: #35, #28, #31, #39, #40, #41
Rebased and cleaned and awaiting my review: #46
Not yet rebased: #47, #42, #43, #44, #48, #49, #50, #45

I also rebased a couple of old topic branches and pushed them to stale/ prefix to clarify that they are old and may be obsolete now. I didn't finish going through all topic branches on the incubator yet, but the plan is to do that.

Gabriel Selzer

@gselzer

Fantastic, many thanks @ctrueden. Did any of the merged PRs need fixes?

2 replies

Gabriel Selzer

@gselzer

@ctrueden thinking about giving the Discoverer a type variable:

public interface Discoverer<T> {
  <U extends T> List<U> discover(Class<U> c);
}

Any opinions?

Curtis Rueden

@ctrueden

@gselzer How does it help?

You'll end up with Discoverer<?> e.g. from the ServiceLoader.

Gabriel Selzer

@gselzer

Yeah, that's kind of what I'm running up against...

I think it would make writing Discoverer implementations much cleaner

But in practice it would be horrible to use...

Curtis Rueden

@ctrueden

It would just avoid the need for a line like:

if (!OpInfo.class.isAssignableFrom(c)) throw new IllegalArgumentException("Cannot discover classes of type " + c.getName());

Gabriel Selzer

@gselzer

Well, I think we'd want a silent failure, but yes

Curtis Rueden

@ctrueden

OK, so return empty list. Sure.

But either way you need to do the check, right?

I think it's cleaner not to have the generic. You can ask any discoverer for any type, but some types they just don't care about.

In which situations would having the type parameter make implementing the Discoverer easier?

Gabriel Selzer

@gselzer

It would be rather nice for implementations of the TherapiDiscoverer implementations

But I might just have to spend more time cleaning that up...

To summarize, TherapiDiscoverer is an abstract class, whose subclasses need to implement (among less important things):

protected abstract <U> U convert(Discovery<AnnotatedElement> e, Class<U> c);

This method is called by

    @Override
    public <U> List<U> discover(Class<U> c) {
        if (!canDiscover(c))
            return Collections.emptyList();
        List<Discovery<AnnotatedElement>> elements = elementsTaggedWith(tagType());
        return convertEach(elements, c);
    }

(I've found great use for Discovery in this one instance, that class will probably be moved into SciJava Discovery Therapi

Curtis Rueden

@ctrueden

Why not put the <T> on the TherapiDiscoverer base class, then?

Gabriel Selzer

@gselzer

I don't think that would help there. We want to say that the U is a T

I'm just going to leave it the way it is

Curtis Rueden

@ctrueden

Sounds good.

Giuseppe Barbieri

@elect86

today I saw this and immediately though at when you, Curtis, told me that the community is kind of hating Java. If you got something light to do, you can hear it in background

Gabriel Selzer

@gselzer

@ctrueden so I think that I am nearly to a state that builds in the discoverer refactoring

The remaining question is how we should give Ops/OpCollections to the StaticDiscoverer in tests

We currently give them classes, but the Discoverers are supposed to give a list of instances

Gabriel Selzer

@gselzer

@ctrueden my goal today is to try and purge SciJava Common, finally

Starting with SciJava Types, we have this NilConverter

Where communities thrive

People

Repo info

Activity