Skip to content

Conversation

orieg
Copy link
Contributor

@orieg orieg commented May 9, 2022

What does this PR do?

This PR add support for the environment variable MLFLOW_FLATTEN_PARAMS.

When first level parameters hold a dictionary value, it will be logged to MLflow as a string. Currently, it will skip that parameter when the string exceed 250 characters. This is especially true with the task_specific_params which can end up being a long string.

Current warning message look like this:

Trainer is attempting to log a value of "{'summarization': {'length_penalty': 1.0, 'max_length': 128, 'min_length': 12, 'num_beams': 4}, 'summarization_cnn': {'length_penalty': 2.0, 'max_length': 142, 'min_length': 56, 'num_beams': 4}, 'summarization_xsum': {'length_penalty': 1.0, 'max_length': 62, 'min_length': 11, 'num_beams': 6}}" for key "task_specific_params" as a parameter. MLflow's log_param() only accepts values no longer than 250 characters so we dropped this attribute.

With this PR, the warning message is updated to:

Trainer is attempting to log a value of "{'summarization': {'length_penalty': 1.0, 'max_length': 128, 'min_length': 12, 'num_beams': 4}, 'summarization_cnn': {'length_penalty': 2.0, 'max_length': 142, 'min_length': 56, 'num_beams': 4}, 'summarization_xsum': {'length_penalty': 1.0, 'max_length': 62, 'min_length': 11, 'num_beams': 6}}" for key "task_specific_params" as a parameter. MLflow's log_param() only accepts values no longer than 250 characters so we dropped this attribute. You can use MLFLOW_FLATTEN_PARAMS environment variable to flatten the parameters and avoid this message.

When a user set the env variable with os.environ['MLFLOW_FLATTEN_PARAMS'] = "True", the parameters will be properly sent to MLflow and logged as such:

task_specific_params.summarization.length_penalty	1.0
task_specific_params.summarization.max_length	128
task_specific_params.summarization.min_length	12
task_specific_params.summarization.num_beams	4
task_specific_params.summarization_cnn.length_penalty	2.0
task_specific_params.summarization_cnn.max_length	142
task_specific_params.summarization_cnn.min_length	56
task_specific_params.summarization_cnn.num_beams	4
task_specific_params.summarization_xsum.length_penalty	1.0
task_specific_params.summarization_xsum.max_length	62
task_specific_params.summarization_xsum.min_length	11
task_specific_params.summarization_xsum.num_beams	6

Before submitting

  • This PR fixes a typo or improves the docs (you can dismiss the other checks if that's the case).
  • Did you read the contributor guideline,
    Pull Request section?
  • Was this discussed/approved via a Github issue or the forum? Please add a link
    to it if that's the case.
  • Did you make sure to update the documentation with your changes? Here are the
    documentation guidelines, and
    here are tips on formatting docstrings.
  • Did you write any new necessary tests?

Who can review?

@sgugger I added a flatten_dict function in .utils/py_utils.py as it didn't seem right to add this in integrations.py.

@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented May 9, 2022

The documentation is not available anymore as the PR was closed or merged.

@orieg
Copy link
Contributor Author

orieg commented May 9, 2022

@sgugger the script utils/check_inits.py used for CI has a bug. Line 252:

submodule = short_path.replace(os.path.sep, ".").replace(".py", "")

This would lead to files starting by "py" to be improperly named. For example, utils/py_utils.py is interpreted as utils_utils.

A better replace() usage may be:

submodule = short_path.replace(".py", "").replace(os.path.sep, ".")

Copy link
Collaborator

@sgugger sgugger left a comment

Choose a reason for hiding this comment

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

Thanks a lot for your PR! Could you move the new flatten_dict function to the utils.generic module? Also could you add a test for it?

Copy link
Collaborator

@sgugger sgugger left a comment

Choose a reason for hiding this comment

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

Thanks for addressing the comments!

@sgugger
Copy link
Collaborator

sgugger commented May 10, 2022

Looks like you messed the rebase a little bit and there are now commits in this PR that shouldn't be here.

@orieg
Copy link
Contributor Author

orieg commented May 10, 2022

Yep. I messed up that rebase...

@sgugger sgugger merged commit e99f0ef into huggingface:main May 10, 2022
@sgugger
Copy link
Collaborator

sgugger commented May 10, 2022

Thanks again!

ArthurZucker pushed a commit to ArthurZucker/transformers that referenced this pull request May 12, 2022
* add support for MLFLOW_FLATTEN_PARAMS

* ensure key is str

* fix style and update warning msg

* Empty commit to trigger CI

* fix bug in check_inits.py

* add unittest for flatten_dict utils

* fix 'NoneType' object is not callable on __del__

* add generic flatten_dict unittest to SPECIAL_MODULE_TO_TEST_MAP

* fix style
elusenji pushed a commit to elusenji/transformers that referenced this pull request Jun 12, 2022
* add support for MLFLOW_FLATTEN_PARAMS

* ensure key is str

* fix style and update warning msg

* Empty commit to trigger CI

* fix bug in check_inits.py

* add unittest for flatten_dict utils

* fix 'NoneType' object is not callable on __del__

* add generic flatten_dict unittest to SPECIAL_MODULE_TO_TEST_MAP

* fix style
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