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

Fix using remote ray actor with external cluster #247

Merged
merged 29 commits into from
Jan 19, 2023

Conversation

AnesBenmerzoug
Copy link
Collaborator

@AnesBenmerzoug AnesBenmerzoug commented Jan 15, 2023

Description

This PR fixes a bug when instantiating RayActorWrapper on an Actor that's present on an external cluster (i.e. one that was not started with ray.init().

Additionally, this modifies the Utility class' handling of warnings so that when using ray with small subsets
we don't end up seeing the same warning message over and over again across workers (e.g. using Naive Bayes with TMC).

Changes

  • Instantiates the worker and coordinator actors directly inside RayActorWrapper.
  • Tests Shapley methods with different parallel backends.
  • Raises an error when attempting use TMC with the sequential backend.
  • Adds a warning to TMC's docstring.
  • Catch all warnings in Utility when show_warnings is set to False

Checklist

  • Wrote Unit tests (if necessary)
  • Updated Documentation (if necessary)
  • Updated Changelog
  • If notebooks were added/changed, added boilerplate cells are tagged with "nbsphinx":"hidden"

@AnesBenmerzoug AnesBenmerzoug self-assigned this Jan 15, 2023
@AnesBenmerzoug AnesBenmerzoug added the bug Something isn't working label Jan 15, 2023
@AnesBenmerzoug AnesBenmerzoug marked this pull request as ready for review January 17, 2023 09:32
Copy link
Collaborator

@mdbenito mdbenito left a comment

Choose a reason for hiding this comment

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

Looks good. I left my usual pesky suggestions and small comments :)

src/pydvl/utils/utility.py Outdated Show resolved Hide resolved
src/pydvl/utils/utility.py Outdated Show resolved Hide resolved
src/pydvl/utils/utility.py Show resolved Hide resolved
src/pydvl/utils/utility.py Outdated Show resolved Hide resolved
src/pydvl/value/shapley/montecarlo.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@mdbenito mdbenito left a comment

Choose a reason for hiding this comment

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

I think there is a little mistake in the tests but I'm going to merge and fix it in my PR if that's ok bc I need to merge in these changes tonight.

Comment on lines +108 to +109
assert u1.signature == u1.signature
assert u2.signature == u2.signature
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this correct?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes. I just wanted to be extra sure since it's a property.
Right now it's redundant though.

):
self.model = model
self.model = self._clone_model(model)
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should document this too: if a clone is expensive, the user must know this will happen

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You're right. Sorry for not doing it.

@mdbenito mdbenito merged commit 52f58b3 into develop Jan 19, 2023
@mdbenito mdbenito deleted the fix-remote-ray-actor-with-external-cluster branch January 19, 2023 17:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants