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

Set port 8786 as default port for scheduler #4431

Closed
wants to merge 9 commits into from

Conversation

FabioRosado
Copy link
Contributor

Hello,

This is my attempt at a fix #4429. I followed Benjamin suggestion and changed the port number in the constructor, I've also added a tiny (silly) test to check if the port is the right one 😄

Let me know if you would like me to change something 🤔

Copy link
Member

@jrbourbeau jrbourbeau left a comment

Choose a reason for hiding this comment

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

Thanks @FabioRosado! Overall this looks good, looking forward to seeing this merged

distributed/deploy/tests/test_local.py Outdated Show resolved Hide resolved
distributed/deploy/tests/test_local.py Outdated Show resolved Hide resolved
@FabioRosado
Copy link
Contributor Author

This last push I added a scheduler_port=0 to the failing tests and they pass on windows. This makes me feel that perhaps we need to do something else to set 8687 as default port, looking at the failing tests I'm not sure what though 🤔

@jrbourbeau
Copy link
Member

Thanks for the updates @FabioRosado, this is looking close! Regarding explicitly passing scheduler_port=0, in some cases (i.e. no explicit scheduler port is requested for and processes is False) we want to automatically select the inproc protocol over tcp. The relevant logic is here:

elif not self.processes and not scheduler_port:
protocol = "inproc://"

Now that we're using 8687 for the default scheduler port, the and not scheduler_port logic is no longer working and we're not setting protocol = "inproc://" as we would like to.

Instead of using 8687 as the default scheduler_port and explicitly passing scheduler_port=0 we might update the default scheduler_port to be None and then add some additional logic to assign it 8687 or 0 depending on whether or we select the tcp or inproc protocol, respectively.

@FabioRosado FabioRosado changed the title Set port 8687 as default port for scheduler Set port 8786 as default port for scheduler Jan 23, 2021
@FabioRosado
Copy link
Contributor Author

I thought about this issue a bit more and the failing test ( test_Client_twice). I think that we could take one of the two approaches here:

  • Decide that the port 8786 is the default one and if we try to run two clients the second one will fail while trying to connect to the default port (so we need to use port=0)
  • Replace the default port to be whatever port=0 assigns and update the code that says that the cluster is being run on 8786

Base automatically changed from master to main March 8, 2021 19:04
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.

LocalCluster not using default ports
2 participants