Add SetTag endpoint

[jescalada]

PR for adding the SetTag endpoint. I also added the DeleteTag endpoint from #8.

There are various new tests, all of which are passing except one:

• PASSING: test_set_tag_with_empty_string_as_value[sqlalchemy]
• PASSING: test_set_tag_validation[sqlalchemy]
• PASSING: test_set_tag_truncate_too_long_tag
• PASSING: test_update_run_name
• PASSING: test_update_run_name_without_changing_status[sqlalchemy]
• FAILING: test_set_tag

The reason why test_set_tag is failing, is that it uses monkeypatch to set an env, which doesn't flow to the Go side in our implementation. I have temporarily disabled that specific test by adding it to conftest.py until we can find a solution for similar tests.

I also added a test:PythonSpecific command for testing a single endpoint at a time.

[jescalada]

Looks like I messed up the line endings, will fix in a sec!

[dsuhinin]

It feels like you need to re generate mocks.

[nojaf]

Hi @jescalada, thanks for this contribution!
There are some interesting changes in here which I did not expect.
It would be great to jump on a call sometime and further discuss.

[jescalada]

I managed to get the test_set_tag_validation test to pass, however, I did this via a manual check in /pkg/tracking/service/tags.go. I also added the custom validator for the runId, however I am not sure how to integrate this with the existing code.

Is this meant to be used in our own Go tests? I noticed that for the LogBatch we're doing custom tests in /pkg/validation/validation_test.go.

[nojaf]

FAILED .mlflow.repo/tests/store/tracking/test_sqlalchemy_store.py::test_set_tag is failing because the test uses monkeypatch.setenv("MLFLOW_TRUNCATE_LONG_VALUES", "true").

@dsuhinin can you give some advice on what should be done in this case.

[nojaf]

Oh and please rebase on latest main.

[nojaf]

Thanks!

[dsuhinin]

It looks good! Left few minor questions/proposals.

[dsuhinin]

So, can we try to have not just an empty test in that case but something like:

run = _run_factory(store)

    tkey = "test tag"
    tval = "a boogie"
    new_val = "new val"
    tag = entities.RunTag(tkey, tval)
    new_tag = entities.RunTag(tkey, new_val)
    store.set_tag(run.info.run_id, tag)
    # Overwriting tags is allowed
    store.set_tag(run.info.run_id, new_tag)
    # test setting tags that are too long fails.
    monkeypatch.setenv("MLFLOW_TRUNCATE_LONG_VALUES", "false")
    with pytest.raises(
        MlflowException, match=f"exceeds the maximum length of {MAX_TAG_VAL_LENGTH} characters"
    ):
        store.set_tag(
            run.info.run_id, entities.RunTag("longTagKey", "a" * (MAX_TAG_VAL_LENGTH + 1))
        )

  
    # test can set tags that are somewhat long
    store.set_tag(run.info.run_id, entities.RunTag("longTagKey", "a" * (MAX_TAG_VAL_LENGTH - 1)))
    run = store.get_run(run.info.run_id)
    assert tkey in run.data.tags
    assert run.data.tags[tkey] == new_val

[dsuhinin]

so without such part which actually failing:

monkeypatch.setenv("MLFLOW_TRUNCATE_LONG_VALUES", "true")
    store.set_tag(run.info.run_id, entities.RunTag("longTagKey", "a" * (MAX_TAG_VAL_LENGTH + 1)))

[jescalada]

@dsuhinin I just tried implementing this, and it seems to require a lot of test-specific dependencies from the MLflow side. For example, the _run_factory function makes use of another helper function _create_experiments and so on, which would clutter our tests if we ported them over. And they are private so I don't think we have a choice other than to implement them directly on our override_test_sqlalchemy_store.py. I feel that implementing these would clutter that file too much, maybe we could have another file for test utils? 🤔 At any rate, I feel that it would be a pretty large change so we could make a separate issue/PR to sort it out!

Edit: I made an issue for it so that we don't forget: #69