这是indexloc提供的服务,不要输入任何密码
Skip to content
This repository was archived by the owner on Feb 16, 2023. It is now read-only.

Conversation

@vvmnnnkv
Copy link
Member

Description

Currently according to README both PyGrid Node and Network use 5000 as default port when started via run.sh, however when started via docker-compose Node uses 300x and Network uses 5000. This leads to situation when model-centric tutorial/demos work or not depending on how PyGrid is started (w/ or w/o docker).
(Additionally, it's not so clear from Network error what's wrong with model-centric API request)
We better assign consistent default ports to PyGrid Node and PyGrid Network so they do not collide.

Affected Dependencies

There're model-centric and grid tutorials in PySyft and demos in KotlinSyft, SwiftSyft, syft.js that use port 5000.
After model-centric API was moved to PyGrid Node, we didn't update ports to 3000 there.
If we put Node to 5xxx and Network to 7xxx, grid tutorials in PySyft needs update because they use 3xxx for Node and 5000 for Network.
But if we put Node on 3xxx and Network on 5000, model-centric tutorials/demos need update.
So both variants are breaking change either for model-centric or grid tutorials.
Please advice which is worse :)
p.s.
Another solution is to make Network to forward model-centric API to Nodes :)

How has this been tested?

Executed Node and Network via run.sh and docker-compose, checked ports.

Checklist

@vvmnnnkv vvmnnnkv added Type: Documentation 📚 Improvements or additions in documentation for some file, feature, or codebase Status: Review Needed 🙋 This needs someone to approve, deny, comment, or request changes Type: Improvement 📈 Performance improvement not introducing a new feature or requiring a major refactor labels Aug 10, 2020
Copy link
Member

@cereallarceny cereallarceny left a comment

Choose a reason for hiding this comment

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

A few naming changes

@vvmnnnkv vvmnnnkv requested a review from cereallarceny August 11, 2020 11:41
@jmaunon
Copy link
Contributor

jmaunon commented Aug 11, 2020

Thanks for your PR. I am working on tutorial and I will clarify this

@vvmnnnkv
Copy link
Member Author

@jmaunon cool, thanks! This PR can be merged together with tutorials then (or you can make it part of your tutorials update).

@cereallarceny cereallarceny changed the title [WIP] Update default ports Update default ports Aug 13, 2020
@cereallarceny cereallarceny merged commit 1ab1463 into dev Aug 13, 2020
@cereallarceny cereallarceny deleted the churn/standardize-ports branch August 13, 2020 10:17
AmrMKayid added a commit that referenced this pull request Aug 14, 2020
* dev:
  [WIP] Update default ports (#691)
AmrMKayid added a commit that referenced this pull request Aug 14, 2020
* dev:
  [WIP] Update default ports (#691)
  Improve websocket performance (#690)
  Create Group model in Node DB (#688)
  Fix dataset search (#687)
  Add support for FSS online key generation for data centric workers (#686)
  Create User model in Network DB (#683)
  Create Role model in Node DB (#684)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Status: Review Needed 🙋 This needs someone to approve, deny, comment, or request changes Type: Documentation 📚 Improvements or additions in documentation for some file, feature, or codebase Type: Improvement 📈 Performance improvement not introducing a new feature or requiring a major refactor

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants