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

Log evaluation results to MLflow #2337

Merged
merged 53 commits into from
Apr 25, 2022
Merged

Log evaluation results to MLflow #2337

merged 53 commits into from
Apr 25, 2022

Conversation

tstadel
Copy link
Member

@tstadel tstadel commented Mar 21, 2022

Proposed changes:

  • add execute_eval_run() to Pipeline which encapsulates pipeline.eval() and logs to MLflow via a MLflowTrackingHead
  • log metadata about pipeline, evalset and corpus
  • remove MLflow version constraint
  • rework MLFlowLogger:
    • introduce Tracker facade that can utilize different TackingHeads by calling Tracker.set_tracking_head()
    • implement MLflowTrackingHead with auto_track_environment param

Usage:

for idx, query_pipeline in enumerate([pipe1, pipe2, pipe3]):
    eval_result = Pipeline.execute_eval_run(
        index_pipeline=index_pipeline,
        query_pipeline=query_pipeline,
        evaluation_set_labels=labels,
        corpus_file_paths=file_paths,
        corpus_file_metas=file_metas,
        experiment_tracking_tool="mlflow",
        experiment_tracking_uri="http://localhost:5000",
        experiment_name="my-query-pipeline-experiment",
        experiment_run_name=f"run_{idx+1}",
        pipeline_meta={"name": f"my-pipeline-{idx+1}"},
        evaluation_set_meta={"name": "my-evalset"},
        corpus_meta={"name": "my-corpus"}.
        add_isolated_node_eval=True,
        reuse_index=False
    )

Status (please check what you already did):

  • First draft (up for discussions & feedback)
  • Final code

closes #2188

@tstadel tstadel requested a review from julian-risch March 23, 2022 13:52
@tstadel tstadel marked this pull request as ready for review March 23, 2022 13:53
@tstadel tstadel marked this pull request as draft March 24, 2022 09:11
Copy link
Member

@julian-risch julian-risch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great draft. I made many comments that are best to discuss in a call I believe.

haystack/modeling/training/base.py Show resolved Hide resolved
haystack/nodes/retriever/dense.py Show resolved Hide resolved
haystack/pipelines/base.py Outdated Show resolved Hide resolved
haystack/pipelines/base.py Outdated Show resolved Hide resolved
haystack/pipelines/base.py Outdated Show resolved Hide resolved
haystack/pipelines/base.py Outdated Show resolved Hide resolved
setup.cfg Show resolved Hide resolved
haystack/utils/experiment_tracking.py Show resolved Hide resolved
haystack/schema.py Outdated Show resolved Hide resolved
haystack/schema.py Outdated Show resolved Hide resolved
@tstadel tstadel marked this pull request as ready for review April 21, 2022 11:29
@julian-risch
Copy link
Member

Seems like the test test_get_all_documents_large_quantities[elasticsearch] is causing problems. I haven't seen that before:

E       elasticsearch.exceptions.TransportError: TransportError(429, 'circuit_breaking_exception', '[parent] Data too large, data for [<http_request>] would be [131020332/124.9mb], which is larger than the limit of [127506841/121.5mb], real usage: [131020280/124.9mb], new bytes reserved: [52/52b], usages [request=0/0b, fielddata=0/0b, in_flight_requests=52/52b, model_inference=0/0b, accounting=10064/9.8kb]')

And mypy:

haystack/pipelines/base.py:964: error: Item "None" of "Optional[BaseDocumentStore]" has no attribute "index"
haystack/pipelines/base.py:965: error: Item "None" of "Optional[BaseDocumentStore]" has no attribute "index"
haystack/pipelines/base.py:966: error: Item "None" of "Optional[BaseDocumentStore]" has no attribute "delete_index"
haystack/pipelines/base.py:966: error: Item "None" of "Optional[BaseDocumentStore]" has no attribute "index"

@tstadel
Copy link
Member Author

tstadel commented Apr 25, 2022

Seems like the test test_get_all_documents_large_quantities[elasticsearch] is causing problems. I haven't seen that before:

E       elasticsearch.exceptions.TransportError: TransportError(429, 'circuit_breaking_exception', '[parent] Data too large, data for [<http_request>] would be [131020332/124.9mb], which is larger than the limit of [127506841/121.5mb], real usage: [131020280/124.9mb], new bytes reserved: [52/52b], usages [request=0/0b, fielddata=0/0b, in_flight_requests=52/52b, model_inference=0/0b, accounting=10064/9.8kb]')

And mypy:

haystack/pipelines/base.py:964: error: Item "None" of "Optional[BaseDocumentStore]" has no attribute "index"
haystack/pipelines/base.py:965: error: Item "None" of "Optional[BaseDocumentStore]" has no attribute "index"
haystack/pipelines/base.py:966: error: Item "None" of "Optional[BaseDocumentStore]" has no attribute "delete_index"
haystack/pipelines/base.py:966: error: Item "None" of "Optional[BaseDocumentStore]" has no attribute "index"

Mypy issues is fixed. The other was probably a glitch in the (elasticsearch) environment. Seems like the scroll of 1000 docs ran out of mem. As this test is in place for quite a while I would mark this as very rare event that probably won't happen again. However we should monitor the tests and if it does happen again we can decrease the batch_size during get_all_documents within this test.

Copy link
Member

@julian-risch julian-risch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! 👍

@julian-risch
Copy link
Member

@tstadel Let's make sure this new feature is properly documented on the website. Maybe you could sync on that topic with @brandenchan ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Logging to MLflow in pipeline.eval()
3 participants