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

add sqlflow as the submodule #70

Closed

Conversation

Yancey1989
Copy link
Collaborator

fixed #69

@@ -2,4 +2,4 @@
test=pytest

[tool:pytest]
rootdir=tests
Copy link
Collaborator Author

@Yancey1989 Yancey1989 Jun 19, 2019

Choose a reason for hiding this comment

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

I removed rootdir because it might not the correct configuration to determine the root director:

look for pytest.ini, tox.ini and setup.cfg files in the ancestor directory and upwards. If one is matched, it becomes the ini-file and its directory becomes the rootdir.

From : https://docs.pytest.org/en/3.0.0/customize.html#initialization-determining-rootdir-and-inifile

@Yancey1989 Yancey1989 requested a review from wangkuiyi June 19, 2019 11:51
Copy link
Collaborator

@typhoonzero typhoonzero left a comment

Choose a reason for hiding this comment

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

LGTM, please @tonyyang-svail have a look at this?

@tonyyang-svail
Copy link
Collaborator

tonyyang-svail commented Jun 20, 2019

I don't think introduce sqlflow in this repo is a good idea. pysqlflow shouldn't depend on sqlflow, since sqlflow are already depending on pysqlflow.

If we want to upgrade the service protocol, we can

  1. sqlflow: Use a fix version of pysqlflow at sqlflow's Docker image https://github.com/sql-machine-learning/sqlflow/blob/develop/scripts/image_build.sh#L37 like sqlflow==0.1.0
  2. pysqlflow: Update pysqlflow's service protocol. Make all tests pass.
  3. pysqlflow: Release a new version of pysqlflow. Simply by make release.
  4. sqlflow: Update sqlflow's service protocol and its Docker image to a new pysqlflow release sqlflow==0.2.0

@typhoonzero
Copy link
Collaborator

I don't think introduce sqlflow in this repo is a good idea. pysqlflow shouldn't depend on sqlflow, since sqlflow are already depending on pysqlflow.

Not sure if only the docker image depends on pysqlflow, if so, it's not likely that it will cause dependency circle. It is an independented docker image used for providing quickstart demos that depends on both sqlflow and pysqlflow, not sqlflow depends on pysqlflow.

Maybe for best convenience, we can move pysqlflow into sqlflow repo as one directory, like most multi language repos, putting go code under pkg/ directory and python code under python/ directory. Then we release pysqlflow to pypi using CI using the same version number as sqlflow, then it will also be easy to find the correct version match.

@tonyyang-svail
Copy link
Collaborator

@typhoonzero Please kindly allow me to explain.

Not sure if only the docker image depends on pysqlflow, if so, it's not likely that it will cause dependency circle. It is an independented docker image used for providing quickstart demos that depends on both sqlflow and pysqlflow, not sqlflow depends on pysqlflow.

sqlflow is responsible for the integration tests between sqlflow and pysqlflow. I consider it as one repo depending on another repo.

Maybe for best convenience, we can move pysqlflow into sqlflow repo as one directory, like most multi language repos, putting go code under pkg/ directory and python code under python/ directory. Then we release pysqlflow to pypi using CI using the same version number as sqlflow, then it will also be easy to find the correct version match.

I am certain combining these two repos makes updating the service protocol easier. However, shouldn't we make this procedure hard so that the service protocol won't be changed easily? Having two repos also enforces the version number change on every service update.

Also, a pure python repo is much easier to test and distribute.

@typhoonzero
Copy link
Collaborator

I am certain combining these two repos makes updating the service protocol easier. However, shouldn't we make this procedure hard so that the service protocol won't be changed easily? Having two repos also enforces the version number change on every service update.

I'm sure there are cases like mysql-server and mysql-connector-python are separated repos, it's a common method used by projects around. And I also believe that when projects become larger, and many client implementations are needed it will be quiet needed to put things in seperated repo for better management.

Well since the current .proto file is tiny and only 1 RPC service is defined, it's cheap to copy it around. I accept both ways, and prefer to merge these two repos since they are both so "small".

@Yancey1989 Yancey1989 closed this Jun 12, 2020
@Yancey1989
Copy link
Collaborator Author

I closed this inactive PR.

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.

To avoid maintain sqlflow.proto file in the separated repo
3 participants