Create get_clearml_task for Ignite.contrib.handlers.ClearMLLogger

[sallycaoyu]

Fixes #2888

Description:

• Added a new method get_clearml_task for the Ignite.contrib.handlers.ClearMLLogger class.
• Wrote and passed test cases locally.

Check list:

• New tests are added (if a new feature is added)
• New doc strings: description and/or example code are in RST format
• Documentation is updated (if required)

[sallycaoyu]

Hello @vfdev-5, for test cases test_clearml_logger_get_clearml_task_not_bypass_create_task and test_clearml_logger_get_clearml_task_not_bypass_task_already_exists I added, I set ClearMLLogger's bypass mode to false. This requires ClearML to be configured on the machine before running the unit tests.

I ran both tests locally and they worked, but they failed the Github checks. The error message says ClearML is not configured on the machine for running GitHub checks. And there doesn't seem to be any unit test that sets bypass mode to False except for the two I mentioned above. But still, I wonder if there is a way to test this scenario by bypassing the ClearML configuration? Any advice is appreciated!

[vfdev-5]

@sallycaoyu thanks for the PR, I'll check in details your above message and answer. Meanwhile, please remove unrelated files from the PR, there 41 files in this PR where majority are your local machine stuff.

[sallycaoyu]

@sallycaoyu thanks for the PR, I'll check in details your above message and answer. Meanwhile, please remove unrelated files from the PR, there 41 files in this PR where majority are your local machine stuff.

@vfdev-5 Thanks for the reply! Have removed the local machine stuff as suggested.

[vfdev-5]

We should use a lazy import here instead. See line 117, https://github.com/pytorch/ignite/pull/2898/files#diff-833e2ffb3025440eee9f6edb419659ce7460b9d263c23186d7c260218c9b8e1fR117

[vfdev-5]

@sallycaoyu I left few comments about the code. Let's only test when bypass is True. Also, please address point 1 from the issue description ("Like TensorboardLogger"), #2888 (comment)

[sallycaoyu]

@vfdev-5 Thank you for your comments! I have addressed point 1 & 2 in #2888. I have changed the method name to get_task, but in fact clearml.Task has Task.get_task() with the same method name. Please let me know if this needs to be further changed to avoid duplicates.

Also, if to test get_task's return value when bypass is True, self._task does not return a Task object, but returns a _Stub object instead. The implementation of the _Stub class makes it unrelated to the Task type, so I cannot call any Task method on self._task to verify it. I also tried building mock objects but they did not work either. Are there any possible ways to test it when bypass is True?

[vfdev-5]

Thanks for the updates @sallycaoyu !

Please let me know if this needs to be further changed to avoid duplicates.

I think it is OK with ClearML.get_task.

Also, if to test get_task's return value when bypass is True, self._task does not return a Task object, but returns a _Stub object instead. The implementation of the _Stub class makes it unrelated to the Task type, so I cannot call any Task method on self._task to verify it. I also tried building mock objects but they did not work either. Are there any possible ways to test it when bypass is True?

It is OK that get_task() returns _Stub. It was a way to implement a testable ClearMLLogger class by ClearML folks. This way there is no external communication to their server and creating a real task. You can just if returned instance is _Stab.

[sallycaoyu]

@vfdev-5 Thanks for the explanation! Have added a test case for get_task.

[vfdev-5]

@sallycaoyu I just checked clearml docs and there is built-in "offline" option: https://clear.ml/docs/latest/docs/clearml_sdk/task_sdk#offline-mode
Maybe, we can use that instead of _Stub and write into a local temp storage. cc @jkhenning

[sallycaoyu]

@vfdev-5 Thanks for the suggestion! I have changed the task in bypass mode from a _Stub to an offline task, for which all data and logs are stored in a temp local folder.

[vfdev-5]

Thanks for the update @sallycaoyu !
Just a minor comment to address and it seems to be good to go

[sallycaoyu]

@vfdev-5 Thanks for the review! I have removed the "return".

[vfdev-5]

LGTM, thanks for the PR @sallycaoyu !

[vfdev-5]

@sallycaoyu there is something strange with CI now. Seems like updated clearml tests from PR influence distributed tests.
Can you try the following locally:

pytest -vvv tests/ignite/contrib/handlers/test_clearml_logger.py tests/ignite/distributed/comp_models/test_native.py

If the execution hangs then we can reproduce the issue.

@jkhenning can you please also check the updated tests and confirm that everything is ok. Thanks

[jkhenning]

@vfdev-5 tests look good to me

[sallycaoyu]

@vfdev-5 In tests/ignite/contrib/handlers/test_clearml_logger.py, I changed the Task object initialization in some unit tests that were

clearml.Task.current_task = Mock(return_value=object())

This initializes the current task to an object(), not of the clearml.Task type, so I changed it to:

clearml.Task.current_task = MagicMock(spec=clearml.Task)

After the change, running the command you provided passed all tests locally.