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

Add SetTag endpoint #10

Merged
merged 27 commits into from
Oct 19, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
41c7754
Add SetTag endpoint
jescalada Oct 4, 2024
97927f6
Fix line endings to LF
jescalada Oct 4, 2024
2cc3ee1
Update mock store
jescalada Oct 8, 2024
59f30ac
Clean up unused code
jescalada Oct 8, 2024
a3235d3
Refactor tests.go
jescalada Oct 8, 2024
b810993
Remove unused validation code
jescalada Oct 8, 2024
084c471
Add SetTag struct validation
jescalada Oct 9, 2024
9039d3f
Replace runId requirement with manual check
jescalada Oct 9, 2024
fd44cbc
Clean up SetTag store
jescalada Oct 9, 2024
4344210
Minor adjustments
jescalada Oct 9, 2024
02ed6d9
Update validations
jescalada Oct 9, 2024
bc25c96
Add preliminary SetTag missing logic
jescalada Oct 11, 2024
7086ea1
Add missing generated file
jescalada Oct 11, 2024
5bc3cd0
Merge remote-tracking branch 'origin/main' into implement-set-tag
jescalada Oct 11, 2024
8eeaacb
Fix protoc version issue
jescalada Oct 11, 2024
91660ed
Fix test_update_run_name test
jescalada Oct 14, 2024
f91e43b
Add pythonSpecific to docs, normalize MLflow spelling
jescalada Oct 14, 2024
a3bd537
Override test_set_tag execution
jescalada Oct 16, 2024
b39b3f0
Merge remote-tracking branch 'origin/main' into implement-set-tag
jescalada Oct 16, 2024
19404be
Extract helper functions from SetTag
jescalada Oct 16, 2024
87c2efe
Revert unnecessary changes
jescalada Oct 16, 2024
a7b7390
Fix postCreate.sh
jescalada Oct 16, 2024
b27e4f6
Fix linter error
jescalada Oct 16, 2024
fe585cc
Fix format issue
jescalada Oct 16, 2024
2f753ad
Simplify handleRunNameUpdate
jescalada Oct 19, 2024
d6081fd
Merge branch 'main' into implement-set-tag
jescalada Oct 19, 2024
069212c
Refactor PythonSpecific
nojaf Oct 19, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 12 additions & 4 deletions CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ to configure all the development environment just run `mage` target:
mage configure
```

it will configure mlflow and all the Python dependencies required by the project or run each step manually:
it will configure MLflow and all the Python dependencies required by the project or run each step manually:

```bash
# Install our Python package and its dependencies
Expand All @@ -17,7 +17,7 @@ pip install -e .
# Install the dreaded psycho
pip install psycopg2-binary

# Archive the MLFlow pre-built UI
# Archive the MLflow pre-built UI
tar -C /usr/local/python/current/lib/python3.8/site-packages/mlflow -czvf ./ui.tgz ./server/js/build

# Clone the MLflow repo
Expand All @@ -30,7 +30,7 @@ tar -C .mlflow.repo/mlflow -xzvf ./ui.tgz
pip install -e .mlflow.repo
```

## Run Go Mlflow server
## Run Go MLflow server

To start the mlflow-go dev server connecting to postgres just run next `mage` target:

Expand Down Expand Up @@ -61,10 +61,18 @@ mage test:all
```

```bash
# Run just MLFlow Python tests
# Run just MLflow Python tests
mage test:python
```

```bash
# Run specific MLflow Python tests (matches all tests containing the argument)
mage test:pythonSpecific <test_file::test_name>

#Example
mage test:pythonSpecific ".mlflow.repo/tests/tracking/test_rest_tracking.py::test_rename_experiment"
```

```bash
# Run just unit tests
mage test:unit
Expand Down
5 changes: 5 additions & 0 deletions conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,11 @@ def pytest_configure(config):
"tests.store.tracking.test_sqlalchemy_store.test_log_param_max_length_value",
"tests/override_test_sqlalchemy_store.py",
),
# This test uses monkeypatch.setenv which does not flow through to the Go side.
(
"tests.store.tracking.test_sqlalchemy_store.test_set_tag",
"tests/override_test_sqlalchemy_store.py",
),
# This tests calls the store using invalid metric entity that cannot be converted
# to its proto counterpart.
# Example: entities.Metric("invalid_metric", None, (int(time.time() * 1000)), 0).to_proto()
Expand Down
2 changes: 1 addition & 1 deletion docs/porting-a-new-endpoint.md
Original file line number Diff line number Diff line change
Expand Up @@ -219,7 +219,7 @@ An example use case where unit tests proved to be highly beneficial is the `filt

## Run Tests

Run `mage test:python` and verify that our Go implementation passes the existing tests.
Run `mage test:python` and verify that our Go implementation passes the existing tests. You can also use `mage test:pythonSpecific <testname>` to run a specific set of tests.

There is one caveat to these tests; occasionally, they may have a Python bias, meaning that the tests pass due to Python's dynamic nature, while our Go tests might fail because they are strongly typed. Another issue may arise if the Python implementation does not consistently return the same error messages. Therefore, it may be necessary to submit a PR to [mlflow](https://github.com/mlflow/mlflow) to adjust the existing tests.

Expand Down
2 changes: 1 addition & 1 deletion magefiles/configure.go
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ func Configure() error {
return err
}

// Archive the MLFlow pre-built UI
// Archive the MLflow pre-built UI
if err := tar(
"-C", "/usr/local/python/current/lib/python3.8/site-packages/mlflow",
"-czvf",
Expand Down
3 changes: 1 addition & 2 deletions magefiles/generate/endpoints.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,9 +26,8 @@ var ServiceInfoMap = map[string]ServiceGenerationInfo{
"logMetric",
"logParam",
"setExperimentTag",
// "setTag",
"setTag",
"setTraceTag",
// "deleteTraceTag",
"deleteTraceTag",
"deleteTag",
"searchRuns",
Expand Down
2 changes: 2 additions & 0 deletions magefiles/generate/validations.go
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,8 @@ var validations = map[string]string{
"SetExperimentTag_ExperimentId": "required",
"SetExperimentTag_Key": "required,max=250,validMetricParamOrTagName",
"SetExperimentTag_Value": "max=5000",
"SetTag_Key": "required,max=1000,validMetricParamOrTagName,pathIsUnique",
"SetTag_Value": "omitempty,truncate=8000",
"LogInputs_RunId": "required,runId",
"LogInputs_Datasets": "required",
"DatasetInput_Dataset": "required",
Expand Down
38 changes: 27 additions & 11 deletions magefiles/tests.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,7 @@ func cleanUpMemoryFile() error {
return nil
}

// Run mlflow Python tests against the Go backend.
func (Test) Python() error {
func runPythonTests(pytestArgs []string) error {
libpath, err := os.MkdirTemp("", "")
if err != nil {
return err
Expand All @@ -45,25 +44,42 @@ func (Test) Python() error {
return nil
}

args := []string{
"--confcutdir=.",
"-k", "not [file",
}
args = append(args, pytestArgs...)

// Run the tests (currently just the server ones)
if err := sh.RunWithV(map[string]string{
"MLFLOW_GO_LIBRARY_PATH": libpath,
}, "pytest",
"--confcutdir=.",
".mlflow.repo/tests/tracking/test_rest_tracking.py",
".mlflow.repo/tests/tracking/test_model_registry.py",
".mlflow.repo/tests/store/tracking/test_sqlalchemy_store.py",
".mlflow.repo/tests/store/model_registry/test_sqlalchemy_store.py",
"-k",
"not [file",
// "-vv",
}, "pytest", args...,
// "-vv",
); err != nil {
return err
}

return nil
}

// Run mlflow Python tests against the Go backend.
func (Test) Python() error {
return runPythonTests([]string{
".mlflow.repo/tests/tracking/test_rest_tracking.py",
".mlflow.repo/tests/tracking/test_model_registry.py",
".mlflow.repo/tests/store/tracking/test_sqlalchemy_store.py",
".mlflow.repo/tests/store/model_registry/test_sqlalchemy_store.py",
})
}

// Run specific Python test against the Go backend.
func (Test) PythonSpecific(testName string) error {
jescalada marked this conversation as resolved.
Show resolved Hide resolved
nojaf marked this conversation as resolved.
Show resolved Hide resolved
return runPythonTests([]string{
testName,
"-vv",
})
}

// Run the Go unit tests.
func (Test) Unit() error {
return sh.RunV("go", "test", "./pkg/...")
Expand Down
5 changes: 5 additions & 0 deletions mlflow_go/store/tracking.py
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
RestoreExperiment,
RestoreRun,
SearchRuns,
SetTag,
SetTraceTag,
UpdateExperiment,
UpdateRun,
Expand Down Expand Up @@ -200,6 +201,10 @@ def delete_trace_tag(self, request_id: str, key: str):
)
self.service.call_endpoint(get_lib().TrackingServiceDeleteTraceTag, request)

def set_tag(self, run_id, tag):
request = SetTag(run_id=run_id, key=tag.key, value=tag.value)
self.service.call_endpoint(get_lib().TrackingServiceSetTag, request)


def TrackingStore(cls):
return type(cls.__name__, (_TrackingStore, cls), {})
Expand Down
1 change: 1 addition & 0 deletions pkg/contract/service/tracking.g.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

8 changes: 8 additions & 0 deletions pkg/lib/tracking.g.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions pkg/protos/service.pb.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

11 changes: 11 additions & 0 deletions pkg/server/routes/tracking.g.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

14 changes: 14 additions & 0 deletions pkg/tracking/service/tags.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,3 +16,17 @@ func (ts TrackingService) DeleteTag(

return &protos.DeleteTag_Response{}, nil
}

func (ts TrackingService) SetTag(ctx context.Context, input *protos.SetTag) (*protos.SetTag_Response, *contract.Error) {
runID := input.GetRunId()

if runID == "" {
runID = input.GetRunUuid()
}

if err := ts.Store.SetTag(ctx, runID, input.GetKey(), input.GetValue()); err != nil {
return nil, err
}

return &protos.SetTag_Response{}, nil
}
51 changes: 51 additions & 0 deletions pkg/tracking/store/mock_tracking_store.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading
Loading