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 expansion of params to logger #91

Merged
merged 22 commits into from
Nov 6, 2024

Conversation

HCookie
Copy link
Member

@HCookie HCookie commented Oct 15, 2024

Closes #69

Expands iterables into dictionary of form {key.i value_i} for mlflow logging

Examples
--------
        >>> expand_iterables({'a': ['a', 'b', 'c']})
        {'a.0': 'a', 'a.1': 'b', 'a.2': 'c', 'a.all': ['a', 'b', 'c'], 'a.length': 3}
        >>> expand_iterables({'a': {'b': ['a', 'b', 'c']}})
        {'a': {'b.0': 'a', 'b.1': 'b', 'b.2': 'c', 'b.all': ['a', 'b', 'c'], 'b.length': 3}}
        >>> expand_iterables({'a': ['a', 'b', 'c']}, size_threshold=100)
        {'a': ['a', 'b', 'c']}
        >>> expand_iterables({'a': [[0,1,2], 'b', 'c']})
        {'a.0': {0: 0, 1: 1, 2: 2}, 'a.1': 'b', 'a.2': 'c', 'a.all': [[0, 1, 2], 'b', 'c'], 'a.length': 3}

@HCookie
Copy link
Member Author

HCookie commented Oct 15, 2024

@MeraX Take a look and see if this addresses the issue you faced.

HCookie and others added 3 commits October 15, 2024 17:38
- Fix issue with duplicated keys
- Will not expand if will fit in logger
@MeraX
Copy link
Contributor

MeraX commented Oct 16, 2024

@HCookie thanks for taking this up. It looks very promising. You are expanding lists/tuples of dicts and dicts recursively. This is fine. I'm wondering if there could be any lists of lists that could be expanded as well. I think list of lists are possible in yaml, but not used in anemoi.

However, I can't test your code it right now, as I have no setup ready that matches the requirements of this branch. Perhaps we could have a quick VC to look at your MLflow output?

The only think I would have chosen differently is the expansion delimiter. I would prefer . over _ as . would expand lists the same way as regular dicts (just with integers keys). Alternatively one could also use f"{key}[{i}]" as a real index notation.

@HCookie HCookie mentioned this pull request Oct 16, 2024
- Add tests
- Alloww configuration of delim, now by default '.'
@HCookie
Copy link
Member Author

HCookie commented Oct 18, 2024

This expansion of params has been noted to cause performance issues with the mlflow servers, so may need to be considered further

- Log all params to artifact
@HCookie HCookie self-assigned this Oct 21, 2024
@mchantry
Copy link
Member

This expansion of params has been noted to cause performance issues with the mlflow servers, so may need to be considered further

Is this because the number of logged parameters increases significantly? I think we saw that MLFlow struggles with this.

@HCookie
Copy link
Member Author

HCookie commented Oct 23, 2024

@mchantry Yeah, increases from 400 -> 4000. when done naively.
With the recent changes that can be brought down to 1200, and further to 500.
Chatting with @anaprietonem, anything over 1000 is to be avoided.
Recent changes with this PR will save all configs anyway but into an artifact.

@HCookie HCookie requested a review from mchantry October 24, 2024 14:12
@HCookie HCookie mentioned this pull request Oct 28, 2024
Copy link
Contributor

@anaprietonem anaprietonem left a comment

Choose a reason for hiding this comment

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

Thanks Harrison! I have left some comments and would be great to see if you have some examples from mlffow runs using this functionality! But thanks so much for the great work!

src/anemoi/training/diagnostics/mlflow/logger.py Outdated Show resolved Hide resolved
src/anemoi/training/diagnostics/mlflow/logger.py Outdated Show resolved Hide resolved
@anaprietonem
Copy link
Contributor

anaprietonem commented Oct 28, 2024

@mchantry Yeah, increases from 400 -> 4000. when done naively. With the recent changes that can be brought down to 1200, and further to 500. Chatting with @anaprietonem, anything over 1000 is to be avoided. Recent changes with this PR will save all configs anyway but into an artifact.

So with just logging the 'config' expanded we would get to 500? or would we still be in 1200? I believe 500 should be fine in terms of server performance

@HCookie
Copy link
Member Author

HCookie commented Oct 29, 2024

@anaprietonem, with only config expanded the number of params approaches 500. This can now be configured by the user, so if they wish to expand more, they can.

anaprietonem
anaprietonem previously approved these changes Oct 29, 2024
Copy link
Contributor

@anaprietonem anaprietonem left a comment

Choose a reason for hiding this comment

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

Approving this PR as Harrison has addressed all the comments and suggestions. This changes should allow to have the parameters better logged without over-stressing the server.

@HCookie HCookie merged commit 9eb68a7 into develop Nov 6, 2024
25 checks passed
@HCookie HCookie deleted the fix/mlflow-log_params-string-truncation branch November 6, 2024 14:26
@MeraX
Copy link
Contributor

MeraX commented Nov 6, 2024

👍

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.

Mlflow log_params string truncation
4 participants