Nothing Special   »   [go: up one dir, main page]

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

Enhancement: Use timestamp returned from the server in QueryResult #529

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

kbolashev
Copy link
Member

Scope of this PR:

  • Fetch the queryDataTime field from the GraphQL of the query, put the value in QueryResult.query_data_time (convert to datetime, assume queryDataTime is a UTC timestamp)
  • Add an as_of argument to Datasource.log_to_mlflow(), that specifies the query timestamp
  • If autologging to MLflow is on (there is an active MLflow run), then QueryResult.query_data_time is passed to the MLflow logging function.
  • Add a DatasourceSerializedState.timed_link field, which has different semantics from DatasourceSerializedState.link:
    • If there is a global as_of already set up on the query, then it is the same as link
    • Otherwise the as_of from the Query is used there (or datetime.now if it was logged by the user with no as_of)

I decided to go the way of adding a new argument to a function so as to not move the whole log_to_mlflow function from Datasource to QueryResult. Considering that we mostly not expect log_to_mlflow to be called by users, and instead they should utilize autologging, this is an OK compromise.

The only UX friction I envision is using Datasource.log_to_mlflow() will now have an additional optional argument, and if the user is using that to save their datasources, they might miss that the time can also be saved there. If you consider that a problem, I can go back to the drawing board and migrate the function to QueryResult.log_to_mlflow

… if autologging.

Add a timed_link arg to DatasourceSerializedState with the url with this as_of set
@kbolashev kbolashev added the enhancement New feature or request label Sep 23, 2024
@kbolashev kbolashev self-assigned this Sep 23, 2024
Copy link
dagshub bot commented Sep 23, 2024

@kbolashev kbolashev changed the base branch from master to pyup-scheduled-update-2020-06-15 October 6, 2024 08:18
@kbolashev kbolashev changed the base branch from pyup-scheduled-update-2020-06-15 to master October 6, 2024 08:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant