Write eval results to delta #10659
Write eval results to delta #10659
Conversation
prithvikannan
added
rn/feature
|
Documentation preview for 57d7d2a will be available here when this CircleCI job completes successfully. More info
|
prithvikannan
changed the title
prithvikannan
added
rn/none
| eval_table_spark.write.mode("overwrite").format("delta").saveAsTable( | ||
| self.eval_results_path | ||
| ) |
There was a problem hiding this comment.
I think this should be an append operation and we should enable schema merge (by setting the spark.databricks.delta.schema.autoMerge.enabled) conf as well.
There was a problem hiding this comment.
We should definitely automerge the schema.
For append vs overwrite:
- Append is the right thing to do if we add a new experiment to the same eval dataset.
- Overwrite is the right thing to do if the eval dataset has changed since last processed.
Can we differentiate between the two cases when the call is made?
There was a problem hiding this comment.
Makes sense! I think we should add an evaluator conf this. Our RAG solution can decide how to populate it
There was a problem hiding this comment.
Sounds good. Introduced a new evaluator conf eval_results_mode to control that.
Signed-off-by: Prithvi Kannan <prithvi.kannan@databricks.com>
Signed-off-by: Prithvi Kannan <prithvi.kannan@databricks.com>
Signed-off-by: Prithvi Kannan <prithvi.kannan@databricks.com>
This reverts commit c16ed30. Signed-off-by: Prithvi Kannan <prithvi.kannan@databricks.com>
Signed-off-by: Prithvi Kannan <prithvi.kannan@databricks.com>
Signed-off-by: Prithvi Kannan <prithvi.kannan@databricks.com>
prithvikannan
force-pushed
the
eval-results-delta
branch
from
41241f3
to
25e311a
Compare
Signed-off-by: Prithvi Kannan <prithvi.kannan@databricks.com>
Signed-off-by: Prithvi Kannan <prithvi.kannan@databricks.com>
Signed-off-by: Prithvi Kannan <prithvi.kannan@databricks.com>
| shutil.rmtree(tmpdir) | ||
|
|
||
|
|
||
| def test_write_to_delta_fails_with_invalid_mode(spark_session_with_tempdir): |
There was a problem hiding this comment.
Seems spark_session_with_tempdir is not used in this test?
There was a problem hiding this comment.
its still passed into this test. the spark session fixture is not tied to the module, so that we can test test_write_to_delta_fails_without_spark above, and then in in this test spark available but an invalid mode.
Signed-off-by: Prithvi Kannan <prithvi.kannan@databricks.com>
| pytest tests/evaluate --ignore=tests/evaluate/test_default_evaluator_delta.py | ||
| - name: Run tests with delta | ||
| run: | | ||
| pytest tests/evaluate/test_default_evaluator_delta.py |
There was a problem hiding this comment.
making this change since the spark sessions from tests/evaluate/test_evaluation and tests/evaluate/test_default_evaluator are not fully isolated (ie. calling getOrCreate() in another file will pickup the spark session from these files).
for tests/evaluate/test_default_evaluator_delta.py we need to test the case with no spark session available, and test with a spark session that has the delta extension.
There was a problem hiding this comment.
How are we testing this with both spark sessions and without it? We are ignoring it in the previous run?
Let me know if I am missing something?
There was a problem hiding this comment.
@sunishsheth2009 , test_default_evaluator_delta has a different fixture to create a spark session with delta. we have a test with and without the fixture.
| "mergeSchema", "true" | ||
| ).format("delta").saveAsTable(self.eval_results_path) | ||
| except Exception as e: | ||
| _logger.info(f"Saving eval table to delta table failed. Reason: {e}") |
There was a problem hiding this comment.
Should we change this to logger.warn instead?
There was a problem hiding this comment.
i dont feel strongly either way, but figured that this might help with debugging during development.
Related Issues/PRs
#xxxWhat changes are proposed in this pull request?
How is this PR tested?
Does this PR require documentation update?
Release Notes
Is this a user-facing change?
What component(s), interfaces, languages, and integrations does this PR affect?
Components
area/artifacts: Artifact stores and artifact loggingarea/build: Build and test infrastructure for MLflowarea/deployments: MLflow Deployments client APIs, server, and third-party Deployments integrationsarea/docs: MLflow documentation pagesarea/examples: Example codearea/model-registry: Model Registry service, APIs, and the fluent client calls for Model Registryarea/models: MLmodel format, model serialization/deserialization, flavorsarea/recipes: Recipes, Recipe APIs, Recipe configs, Recipe Templatesarea/projects: MLproject format, project running backendsarea/scoring: MLflow Model server, model deployment tools, Spark UDFsarea/server-infra: MLflow Tracking server backendarea/tracking: Tracking Service, tracking client APIs, autologgingInterface
area/uiux: Front-end, user experience, plotting, JavaScript, JavaScript dev serverarea/docker: Docker use across MLflow's components, such as MLflow Projects and MLflow Modelsarea/sqlalchemy: Use of SQLAlchemy in the Tracking Service or Model Registryarea/windows: Windows supportLanguage
language/r: R APIs and clientslanguage/java: Java APIs and clientslanguage/new: Proposals for new client languagesIntegrations
integrations/azure: Azure and Azure ML integrationsintegrations/sagemaker: SageMaker integrationsintegrations/databricks: Databricks integrationsHow should the PR be classified in the release notes? Choose one:
rn/none- No description will be included. The PR will be mentioned only by the PR number in the "Small Bugfixes and Documentation Updates" sectionrn/breaking-change- The PR will be mentioned in the "Breaking Changes" sectionrn/feature- A new user-facing feature worth mentioning in the release notesrn/bug-fix- A user-facing bug fix worth mentioning in the release notesrn/documentation- A user-facing documentation change worth mentioning in the release notes