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

Try to fix experiment already exist issue in MLFlowHandler #7916

Merged
merged 7 commits into from
Jul 18, 2024

Conversation

KumoLiu
Copy link
Contributor

@KumoLiu KumoLiu commented Jul 14, 2024

Try to fixes NVIDIA/NVFlare#2698.

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.

Signed-off-by: YunLiu <55491388+KumoLiu@users.noreply.github.com>
@binliunls
Copy link
Contributor

I think one more thing to discuss is whether MONAI should handle the multiprocessing scene of the mlflow handler or users should implement it in their code. If we'd like to do this, we'd better to use a more formal way like a lock for this.

Thanks
Bin

KumoLiu added 2 commits July 16, 2024 15:09
Signed-off-by: YunLiu <55491388+KumoLiu@users.noreply.github.com>
@KumoLiu
Copy link
Contributor Author

KumoLiu commented Jul 16, 2024

I think one more thing to discuss is whether MONAI should handle the multiprocessing scene of the mlflow handler or users should implement it in their code. If we'd like to do this, we'd better to use a more formal way like a lock for this.

A good point. I didn't find a multiprocessing scene, the issue here is that in the nvflare real world tests, it will raise an error during creating experiments in different sites. Perhaps we can add try-catch block here and if there is a real-world case, we can refactor the logic. What do you think? Thanks.

@binliunls
Copy link
Contributor

I think one more thing to discuss is whether MONAI should handle the multiprocessing scene of the mlflow handler or users should implement it in their code. If we'd like to do this, we'd better to use a more formal way like a lock for this.

A good point. I didn't find a multiprocessing scene, the issue here is that in the nvflare real world tests, it will raise an error during creating experiments in different sites. Perhaps we can add try-catch block here and if there is a real-world case, we can refactor the logic. What do you think? Thanks.

Sure, we can leave this one as it is. But I think from the flare side, they need to handle this multiprocessing issue.

KumoLiu added 2 commits July 17, 2024 13:26
Signed-off-by: YunLiu <55491388+KumoLiu@users.noreply.github.com>
@KumoLiu
Copy link
Contributor Author

KumoLiu commented Jul 17, 2024

/build

4 similar comments
@KumoLiu
Copy link
Contributor Author

KumoLiu commented Jul 17, 2024

/build

@KumoLiu
Copy link
Contributor Author

KumoLiu commented Jul 17, 2024

/build

@KumoLiu
Copy link
Contributor Author

KumoLiu commented Jul 17, 2024

/build

@Nic-Ma
Copy link
Contributor

Nic-Ma commented Jul 17, 2024

/build

@KumoLiu
Copy link
Contributor Author

KumoLiu commented Jul 18, 2024

/build

1 similar comment
@YanxuanLiu
Copy link
Collaborator

/build

@KumoLiu
Copy link
Contributor Author

KumoLiu commented Jul 18, 2024

/build

@KumoLiu KumoLiu enabled auto-merge (squash) July 18, 2024 06:45
@KumoLiu KumoLiu merged commit 85ab9f4 into Project-MONAI:dev Jul 18, 2024
28 checks passed
@KumoLiu KumoLiu deleted the nvflare-mlflow branch July 18, 2024 07:03
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.

[BUG] RESOURCE_ALREADY_EXISTS: Experiment 'monai_nvflare' already exists.
6 participants