Skip to content

Conversation

@Nic-Ma
Copy link
Contributor

@Nic-Ma Nic-Ma commented Dec 6, 2022

Fixes #5648 .

Description

This PR added support to track the executed bundle config in the MLFlow, also fixed #4057 (comment).

Types of changes

  • Non-breaking change (fix or new feature that would not break existing functionality).
  • Breaking change (fix or new feature that would cause existing functionality to change).
  • New tests added to cover the changes.
  • Integration tests passed locally by running ./runtests.sh -f -u --net --coverage.
  • Quick tests passed locally by running ./runtests.sh --quick --unittests --disttests.
  • In-line docstrings updated.
  • Documentation updated, tested make html command in the docs/ folder.

@Nic-Ma
Copy link
Contributor Author

Nic-Ma commented Dec 6, 2022

Hi @wyli , @binliunls ,

I tried to track the executed bundle config as a string param in MLFlow, but unfortunately MLFlow has some limitation:

mlflow.exceptions.MlflowException: Param value *** had length 2005, which exceeded length limit of 500

So I plan to save the executed bundle config in a file and put it as artifacts.
What do you think?

Thanks in advance.

@wyli
Copy link
Contributor

wyli commented Dec 6, 2022

agreed, perhaps that's better because we can track timestamp and it's reusable...

Signed-off-by: Nic Ma <nma@nvidia.com>
Signed-off-by: Nic Ma <nma@nvidia.com>
Signed-off-by: Nic Ma <nma@nvidia.com>
@Nic-Ma Nic-Ma requested review from binliunls and wyli December 7, 2022 11:43
@Nic-Ma
Copy link
Contributor Author

Nic-Ma commented Dec 7, 2022

/build

Signed-off-by: Nic Ma <nma@nvidia.com>
@Nic-Ma
Copy link
Contributor Author

Nic-Ma commented Dec 7, 2022

/build

@Nic-Ma Nic-Ma marked this pull request as ready for review December 7, 2022 16:23
@Nic-Ma Nic-Ma changed the title [WIP] 5648 track bundle config in MLFlow 5648 track bundle config in MLFlow Dec 7, 2022
@Nic-Ma Nic-Ma requested a review from binliunls December 7, 2022 16:23
@wyli wyli enabled auto-merge (squash) December 7, 2022 17:22
@wyli
Copy link
Contributor

wyli commented Dec 7, 2022

/build

@wyli
Copy link
Contributor

wyli commented Dec 7, 2022

/integration-test
/build

@wyli wyli disabled auto-merge December 7, 2022 21:34
@Nic-Ma
Copy link
Contributor Author

Nic-Ma commented Dec 7, 2022

Windows test error:

Traceback (most recent call last):
  File "C:\hostedtoolcache\windows\Python\3.8.10\x64\lib\site-packages\parameterized\parameterized.py", line 533, in standalone_func
    return func(*(a + p.args), **p.kwargs)
  File "D:\a\MONAI\MONAI\tests\test_integration_bundle_run.py", line 120, in test_shape
    command_line_tests(la + ["--args_file", def_args_file] + ["--tracking", settings_file])
  File "D:\a\MONAI\MONAI\tests\utils.py", line 776, in command_line_tests
    raise RuntimeError(f"subprocess call error {e.returncode}: {errors}, {output}") from e
RuntimeError: subprocess call error 1: b'Traceback (most recent call last):\r
  File "D:\\a\\MONAI\\MONAI\\monai\\bundle\\__main__.py", line 18, in <module>\r
    fire.Fire()\r
  File "C:\\hostedtoolcache\\windows\\Python\\3.8.10\\x64\\lib\\site-packages\\fire\\core.py", line 141, in Fire\r
    component_trace = _Fire(component, args, parsed_flag_args, context, name)\r
  File "C:\\hostedtoolcache\\windows\\Python\\3.8.10\\x64\\lib\\site-packages\\fire\\core.py", line 466, in _Fire\r
    component, remaining_args = _CallAndUpdateTrace(\r
  File "C:\\hostedtoolcache\\windows\\Python\\3.8.10\\x64\\lib\\site-packages\\fire\\core.py", line 681, in _CallAndUpdateTrace\r
    component = fn(*varargs, **kwargs)\r
  File "D:\\a\\MONAI\\MONAI\\monai\\bundle\\scripts.py", line 638, in run\r
    patch_bundle_tracking(parser=parser, settings=settings_)\r
  File "D:\\a\\MONAI\\MONAI\\monai\\bundle\\scripts.py", line 500, in patch_bundle_tracking\r
    Path(filepath).parent.mkdir(parents=True, exist_ok=True)\r
  File "C:\\hostedtoolcache\\windows\\Python\\3.8.10\\x64\\lib\\pathlib.py", line 1288, in mkdir\r
    self._accessor.mkdir(self, mode)\r
OSError: [WinError 123] The filename, directory name, or volume label syntax is incorrect: \'file:\\\\C:\\\\Users\\\\RUNNER~1\\\\AppData\\\\Local\\\\Temp\\\	mp7nzhmvkh\'\r
', b"2022-12-07 20:47:06,896 - INFO - --- input summary of monai.bundle.scripts.run ---\r

Let me update the path format.

Thanks.

Signed-off-by: Nic Ma <nma@nvidia.com>
@Nic-Ma
Copy link
Contributor Author

Nic-Ma commented Dec 8, 2022

/black

@Nic-Ma
Copy link
Contributor Author

Nic-Ma commented Dec 8, 2022

/build

@Nic-Ma
Copy link
Contributor Author

Nic-Ma commented Dec 8, 2022

Windows issue fixed.

Thanks.

@Nic-Ma Nic-Ma requested a review from wyli December 8, 2022 02:36
@wyli
Copy link
Contributor

wyli commented Dec 8, 2022

Windows issue fixed.

Thanks.

do you know what the root cause is?

@Nic-Ma
Copy link
Contributor Author

Nic-Ma commented Dec 8, 2022

Windows issue fixed.
Thanks.

do you know what the root cause is?

The root cause is that the previous test code changed output_dir from regular path to URI on windows:
https://github.com/Project-MONAI/MONAI/blob/dev/tests/test_integration_bundle_run.py#L108
Then can't use it to dump the config content to a file.
Already updated in the latest commit.

Thanks.

Signed-off-by: Nic Ma <nma@nvidia.com>
@Nic-Ma
Copy link
Contributor Author

Nic-Ma commented Dec 8, 2022

/black

@Nic-Ma
Copy link
Contributor Author

Nic-Ma commented Dec 8, 2022

/build

@Nic-Ma Nic-Ma requested a review from wyli December 8, 2022 11:19
@wyli wyli enabled auto-merge (squash) December 8, 2022 12:16
@wyli
Copy link
Contributor

wyli commented Dec 8, 2022

/build

@wyli wyli merged commit 78d4f42 into Project-MONAI:dev Dec 8, 2022
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.

Add support to record the executed bundle config content in MLFlow Support string config starts with bundle syntax or expression contains "@"

3 participants