Extractors -> listeners, trigger listeners on saved feeds automatically #114
Conversation
max-zilla
changed the title
max-zilla
changed the title
tcnichol
left a comment
tcnichol
left a comment
There was a problem hiding this comment.
Ran this one and it still works with extractors. Looks good. Marking approved.
|
I think the failing test might be due to ListenerIn needing to be a superclass of ExtractorInfo instead of BaseModel, or perhaps the other way around. |
lmarini
left a comment
lmarini
left a comment
There was a problem hiding this comment.
Can you add docstrings to all functions and models? We need to start requiring this on all PRs. Thanks!
|
Will you be adding https://github.com/clowder-framework/clowder2/blob/workflow-documentation/backend/docs/source/listeners.md to this PR? |
|
Are you still in favor or renaming |
Added a bunch of these.
Added with minor updates as well.
I renamed to EventListeners across the app, although "listeners.py" is still the filename for simplicity. |
lmarini
left a comment
lmarini
left a comment
There was a problem hiding this comment.
Thanks for updating the PR. Left a few more questions / comments as I continue to review this. There is a lot of digest. Thanks!
| parameters: List[dict] = [] | ||
|
|
||
|
|
||
| class EventListenerBase(BaseModel): |
There was a problem hiding this comment.
Is this supposed to inherit from ExtractorInfo?
There was a problem hiding this comment.
no, my intent was to allow listeners to be registered without requiring v1 extractor info. So submitting a v2 listener to /api/listeners doesn't need extractor_info contents, but /api/extractors does.
more broadly my hope was that we would rethink/redesign extractor_info because it has some limitations/bloat. however there is some tension between doing that and getting a working demo sooner with an existing extractor. if it is all too confusing, we can just remove LegacyListener concept and revert v2 models to match v1 and keep extractor/listener framework more or less the same? not sure what is easiest for short vs. long term.
There was a problem hiding this comment.
Sounds good. We can try to keep both. Maybe add a few more comments explaining the difference? As long as we can submit/trigger with current extractors, we are good short term. I don't think this gets in the way of that?
| """v1 Extractors can submit data formatted as a LegacyEventListener (i.e. v1 format) and it will be converted to a v2 EventListener.""" | ||
|
|
||
| name: str | ||
| version: str = "1.0" |
There was a problem hiding this comment.
Why the difference in types between version for EventListenerBase and LegacyEventListener (str vs int)?
There was a problem hiding this comment.
Clowder v1 lets users arbitrarily provide a version with no rules (e.g. "1", "1.0", "v1" "beta" all allowed). I was thinking (before v1 back-compatibility) that version could just be an integer for clarity and simplicity, but after seeing it is just a string field for v1 we should probably use that.
If we use string, "version" might better be called "tag" or something like Docker does? Don't necessarily see a point in enforcing something that can be cast as a float in that case...
There was a problem hiding this comment.
Clowder v1 lets users arbitrarily provide a version with no rules (e.g. "1", "1.0", "v1" "beta" all allowed). I was thinking (before v1 back-compatibility) that version could just be an integer for clarity and simplicity, but after seeing it is just a string field for v1 we should probably use that.
If we use string, "version" might better be called "tag" or something like Docker does? Don't necessarily see a point in enforcing something that can be cast as a float in that case...
Good point. Tag sounds great. We could also enforce semantic versioning in the string and call it version. But that might be more work. Similar to what npm does.
| os.remove(dummy_file) | ||
| assert response.status_code == 200 | ||
|
|
||
| # Verify the message |
There was a problem hiding this comment.
Is there a way to test that the listener was triggered?
There was a problem hiding this comment.
not yet. if we had an event log we could check that but it doesn't exist yet. Otherwise we'd have to check RMQ somehow.
There was a problem hiding this comment.
@tcnichol how easily could you add v1 events back into v2? I am looking at the heartbeat PR and will leave comments there soon. It would be a similar approach?
backend/app/routers/feeds.py
Outdated
|
|
||
| router = APIRouter() | ||
|
|
||
| clowder_bucket = os.getenv("MINIO_BUCKET_NAME", "clowder") |
There was a problem hiding this comment.
I don't think this is being used.
backend/app/routers/feeds.py
Outdated
| listeners_found.append(listener.listener_id) | ||
|
|
||
| for targ_listener in listeners_found: | ||
| queue = "" # TODO: Each extractor gets a queue - routing key same as name? |
There was a problem hiding this comment.
Can this default to the listener name so that legacy extractors can be triggered?
This renames extractors to listeners (and creates a legacy endpoint for old extractors) as well as includes basic implementation of job feeds (saved searches) and listeners on those feeds. Lots of decisions here we should discuss.
See test_feeds.py for implementation example, still in progress and very rough.