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

Improve documentation clarity for tags variable and update type in function definition. #52

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

Conversation

vonpetersenn
Copy link

Summary:

  • Clarified the expected structure and content of the tags variable in the documentation.

Changes:

  • Initially changed the type declaration of tags in the first commit, which caused errors in the test.
  • Reverted the type declaration to maintain consistency with the library's style.

Notes:

  • The type declaration was omitted to avoid test errors and adhere to the existing style of the library.

- Clarified the expected structure and content of the `tags` variable in the documentation.
@@ -7,7 +7,8 @@ Creates an MLFlow experiment.
- `mlf`: [`MLFlow`](@ref) configuration.
- `name`: experiment name. If not specified, MLFlow sets it.
- `artifact_location`: directory where artifacts of this experiment will be stored. If not specified, MLFlow uses its default configuration.
- `tags`: a Dictionary of key-values which tag the experiment.
- `tags`: a Vector of Dictionaries which tag the experiment.
Copy link
Member

Choose a reason for hiding this comment

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

@pebeto Is this really the way this works?

Copy link
Member

Choose a reason for hiding this comment

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

Hi @ablaom. Yes, tags must be a Vector of Dictionaries with key and value, as expected in mlflow (i.e. https://mlflow.org/docs/latest/rest-api.html?highlight=tags#experimenttag).

Copy link
Member

@pebeto pebeto Jun 20, 2024

Choose a reason for hiding this comment

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

Also is good to mention that I'm working on the next big change for this library (9fe5e9a), where I'm pairing everything with the last mlflow specification (including types).

Copy link
Member

@ablaom ablaom Jun 20, 2024

Choose a reason for hiding this comment

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

This just seems weird. If I want to assign the value "bar" to a tag called "foo", and a value "gala" to a tag called "missy", I would have expected to pass Dict("foo" => "bar", "missy" => "gala"). But my reading of the proposed doc clarification is different: I should pass [Dict("key" => "foo", "value" => "bar"), Dict("key" => "missy", "value" => "gala")] instead?

Copy link
Author

Choose a reason for hiding this comment

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

I also found it counterintuitive, which is why I opened the pull request.

Copy link
Member

Choose a reason for hiding this comment

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

Right. Then I'll leave to @pebeto to finalise the review. Perhaps we can make this more intuitive at the next breaking release.

@ablaom
Copy link
Member

ablaom commented Jul 19, 2024

@pebeto Do you have the possiblity to finish this review?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants