Skip to content

Conversation

teng-li
Copy link
Contributor

@teng-li teng-li commented Nov 15, 2018

This will be super helpful to the user

@teng-li teng-li requested a review from pietern November 15, 2018 19:55
@teng-li teng-li added the oncall: distributed Add this issue/PR to distributed oncall triage queue label Nov 15, 2018
@pietern
Copy link
Contributor

pietern commented Nov 15, 2018

The version check already happens in __init__.py. You can move this assertion into the else branch of that version check and define a spawn function there that just raises.

@pietern
Copy link
Contributor

pietern commented Nov 15, 2018

The else branch of this one:

if sys.version_info >= (3, 4):
"""Add helper function to spawn N processes and wait for completion of any of
them. This depends `mp.get_context` which was added in Python 3.4."""
from .spawn import spawn, SpawnContext

@teng-li
Copy link
Contributor Author

teng-li commented Nov 15, 2018

@pietern Not a big fan of creating another dummy class for this purpose. Changed to check the version at runtime and we always import these classes.

from .spawn import spawn, SpawnContext
"""Add helper function to spawn N processes and wait for completion of any of
them. This depends `mp.get_context` which was added in Python 3.4."""
from .spawn import spawn, SpawnContext

This comment was marked as off-topic.

This comment was marked as off-topic.


class SpawnContext:
def __init__(self, processes, error_queues):
_python_version_check()

This comment was marked as off-topic.

This comment was marked as off-topic.

if sys.version_info < (3, 4):
raise RuntimeError("Requires python 3.4 or higher to use "
"torch.multiprocessing.spawn and "
"torch.multiprocessing.SpawnContext helper "

This comment was marked as off-topic.

This comment was marked as off-topic.

@pietern
Copy link
Contributor

pietern commented Nov 15, 2018

I didn't imply another class, just:

def spawn(*args, **kwargs):
  raise RuntimeError("Python >= 3.4 required bla bla bla")

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@teng-li is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@pietern
Copy link
Contributor

pietern commented Nov 15, 2018

Here's the conundrum: nobody should be using the SpawnContext directly, but its join function should show up in the docs, because that's how you end up doing checking, waiting, and finally ensuring that all processes terminate. Mentioning the existence of some unusable SpawnContext in the error message only adds confusion IMO.

Two possible solutions:

  1. No more SpawnContext: This would be some anonymous object with a join function, that we can somehow include in the documentation (no idea how).
  2. More SpawnContext: Make it usable by itself and turn spawn into a simple factory function.

The latter is a bit better because we don't have to worry about the docs and the class is usable by itself.

Filed #14047. No need to block this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

oncall: distributed Add this issue/PR to distributed oncall triage queue

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants