-
Notifications
You must be signed in to change notification settings - Fork 0
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
Changing from a plugin to a standalone CLI application #68
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: jzonthemtn <[email protected]>
Signed-off-by: jzonthemtn <[email protected]>
Signed-off-by: jzonthemtn <[email protected]>
Signed-off-by: jzonthemtn <[email protected]>
Signed-off-by: jzonthemtn <[email protected]>
Signed-off-by: jzonthemtn <[email protected]>
Signed-off-by: jzonthemtn <[email protected]>
Signed-off-by: jzonthemtn <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left comments regarding pipeline names in the scripts.
I tested calculating judgments with Chorus and got a bunch of errors connected to the JSON structure and fields of the UBI events and queries. I did some local changes and fixed the findings but I think it makes sense to look at them together to make sure we make changes in the right place (here vs Chorus vs the specification vs the UBI JS client).
Other findings:
- Running the query set generator with a
querySetSize
of 5 resulted in 6 queries in the query set. - Running a query set reported a successful run (
Query set run complete: ...
). However there were no calculated metrics stored.
Let's have a look together.
@@ -7,4 +7,4 @@ echo "Initializing UBI..." | |||
curl -s -X POST "http://localhost:9200/_plugins/ubi/initialize" | |||
|
|||
echo "Indexing queries and events..." | |||
curl -s -T "http://localhost:9200/_bulk?pretty" -H "Content-Type: application/x-ndjson" --data-binary @ubi_queries_events.ndjson | |||
curl -X POST 'http://localhost:9200/index-name/_bulk?pretty' --data-binary @ubi_queries_events.ndjson -H "Content-Type: application/x-ndjson" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does that work with index-name
?
I'd expect the request to be curl -X POST 'http://localhost:9200/index-name/_bulk?pretty' --data-binary @ubi_queries_events.ndjson -H "Content-Type: application/x-ndjson"
and the index names are taken from the requests in the ndjson
file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does work. Probably a nuance of OpenSearch. The index-name
is ignored by OpenSearch. But I agree it's not clear to look at.
scripts/create-pipeline.sh
Outdated
@@ -0,0 +1,26 @@ | |||
#!/bin/bash -e | |||
|
|||
curl -X PUT http://localhost:9200/_search/pipeline/hybrid_pipeline -H "Content-type: application/json" -d' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest we rename the pipeline to represent its functionality, for example filter_pipeline
.
scripts/get-models.sh
Outdated
#!/bin/bash -e | ||
|
||
# Get the search pipeline. | ||
curl -s http://localhost:9200/_search/pipeline/hybrid-search-pipeline | jq |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest we align this pipeline name with the name of the pipeline created in scripts/create-pipeline.sh
scripts/queryset.json
Outdated
"query_set_id": "20b2c5d3-42bf-4aaf-843e-d806e9fcd3c1", | ||
"judgments_id": "69a69c21-f0bb-443e-bf75-bca389704fd0", | ||
"index": "ecommerce", | ||
"search_pipeline": "hybrid_pipeline", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest we rename the pipeline to represent its functionality, for example filter_pipeline.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated the pipeline name.
Found and fixed this one. |
I got the metrics indexing now. I had that part removed for a bit. (doh) |
Signed-off-by: jzonthemtn <[email protected]>
Signed-off-by: jzonthemtn <[email protected]>
Signed-off-by: jzonthemtn <[email protected]>
Changing from a plugin to a standalone CLI application.