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

Migrating log-param endpoint. #7

Merged
merged 5 commits into from
Oct 7, 2024

Conversation

dsuhinin
Copy link
Collaborator

@dsuhinin dsuhinin commented Oct 1, 2024

No description provided.

Signed-off-by: Software Developer <[email protected]>
@dsuhinin dsuhinin marked this pull request as ready for review October 2, 2024 06:57
@dsuhinin dsuhinin requested review from nojaf and jgiannuzzi October 2, 2024 06:58
Copy link
Collaborator

@nojaf nojaf left a comment

Choose a reason for hiding this comment

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

Wait, does this not need a change to tracking.py as well?

@dsuhinin
Copy link
Collaborator Author

dsuhinin commented Oct 2, 2024

Wait, does this not need a change to tracking.py as well?

yes, Im trying to figure out why this file is not committed.

Signed-off-by: Software Developer <[email protected]>
Signed-off-by: Software Developer <[email protected]>
Signed-off-by: Software Developer <[email protected]>
Signed-off-by: Software Developer <[email protected]>
@dsuhinin dsuhinin requested a review from nojaf October 2, 2024 16:31
@@ -4,19 +4,26 @@ import "github.com/mlflow/mlflow-go/pkg/protos"

type Param struct {
Key string
Value string
Value *string
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

MLflow has a special test here to send None so it seems like we can handle it only such way.

Value string `db:"value" gorm:"column:value;not null"`
RunID string `db:"run_uuid" gorm:"column:run_uuid;primaryKey"`
Key string `db:"key" gorm:"column:key;primaryKey"`
Value sql.NullString `db:"value" gorm:"column:value;not null"`
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Again, to correctly handle MLFlow None case.

Copy link
Collaborator

Choose a reason for hiding this comment

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

image

Should we not discuss this with the mlflow team?
Database schema says it is not null, we even have it in the gorm annotation.

return contract.NewErrorWith(
protos.ErrorCode_INTERNAL_ERROR,
fmt.Sprintf("error creating params in batch for run_uuid %q", runID),
protos.ErrorCode_BAD_REQUEST,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is the most interesting part. MLFlow test waits for such a status and Im not sure that we can fix it on theirs side or suggest something.

Copy link
Collaborator

@nojaf nojaf left a comment

Choose a reason for hiding this comment

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

I think this is fine, but weird stuff over at mlflow.
I'm ok with merging this, but please open an issue over at mlflow so we can follow up if they agree,

Value string `db:"value" gorm:"column:value;not null"`
RunID string `db:"run_uuid" gorm:"column:run_uuid;primaryKey"`
Key string `db:"key" gorm:"column:key;primaryKey"`
Value sql.NullString `db:"value" gorm:"column:value;not null"`
Copy link
Collaborator

Choose a reason for hiding this comment

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

image

Should we not discuss this with the mlflow team?
Database schema says it is not null, we even have it in the gorm annotation.

@dsuhinin dsuhinin merged commit 7e65622 into mlflow:main Oct 7, 2024
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants