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

chore(ci/cd): add pipelines + modernize repo #193

Merged
merged 24 commits into from
May 15, 2023
Merged

Conversation

joshua-goldstein
Copy link
Contributor

@joshua-goldstein joshua-goldstein commented May 11, 2023

This PR adds a CD pipeline for pydgraph. We also bring the repo into the modern era to support Python 3.11, and use modern build tools. Some noteworthy points:

  • python setup.py [command] is being deprecated (see here e.g.).
  • The correct approach now is to use "front end" build tools like python -m build and pip install -e . instead of invoking setup.py directly
  • We introduce a pyproject.toml file, which deprecates setup.py, requirements.txt, setup.cfg, etc.
  • We remove requirements.txt since this information can be found in pyproject.toml
  • Dev requirements introduced as optional dependencies in pyproject.toml (use pip install .[dev] from local repo, or pip install pydgraph[dev] once it is pushed to pypi)
  • Test framework is unified (i.e. local tests and CI tests use the same script local-test.sh)
  • README and PUBLISHING updated to reflect the above changes

Copy link
Contributor

@mangalaman93 mangalaman93 left a comment

Choose a reason for hiding this comment

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

I like this method of setting up the cluster more reliable. Maybe we should consider doing this for dgo too.

README.md Outdated
@@ -63,6 +53,7 @@ use a different version of this client.
| 1.0.X | <= *1.2.0* |
| 1.1.X | >= *2.0.0* |
| 1.2.X | >= *2.0.0* |
| 23.X.Y | >= *23.0.0* |
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't we need to follow calver for pydgraph. We should just do a minor release instead

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree it is not clean but unfortunately there are already calver releases for pydgraph: https://github.com/dgraph-io/pydgraph/releases

The latest release is v21.03.0. The table here was not updated.

README.md Show resolved Hide resolved
README.md Outdated
This script assumes Dgraph and [dgo](https://github.com/dgraph-io/dgo) (Go
client) are already built on the local machine and that their code is in
`$GOPATH/src`. It also requires that docker and docker-compose are installed in
This script assumes dgraph is located on your path. Dgraph release binaries can be found [here](https://github.com/dgraph-io/dgraph/releases).
Copy link
Contributor

Choose a reason for hiding this comment

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

limit the line width

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -0,0 +1,5 @@
build==0.10.0
Copy link
Contributor

Choose a reason for hiding this comment

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

why not requirements.txt?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are two "use-cases" so to speak:

  • If I am a developer of an application that only needs to import pydgraph, when I run pip install pydgraph it will pull the needed dependencies (grpcio, protobuf). Also, these dependencies are now captured by the pyproject.toml file. There should be only one source of truth and we should not keep the same information in two places (unless necessary).
  • If I am working on developing pydgraph, then I will need certain build / testing tooling, so I created this requirements_dev file for exactly those dependencies.

@mangalaman93
Copy link
Contributor

Thanks for cleaning up the repo. Should we mention somewhere that we don't support python 2 anymore?

@joshua-goldstein
Copy link
Contributor Author

Thanks for cleaning up the repo. Should we mention somewhere that we don't support python 2 anymore?

No problem. This is mentioned in the pyproject.toml file already, I think that should be sufficient.

@matthewmcneely
Copy link
Contributor

@joshua-goldstein Timely! I was struggling getting the testing (tox) working this week. Is there a plan to move to a more simple setup (basic unittest for instance)?

@mangalaman93 mangalaman93 merged commit 323a877 into master May 15, 2023
@mangalaman93 mangalaman93 deleted the joshua/cleanup branch May 15, 2023 15:39
@joshua-goldstein
Copy link
Contributor Author

@joshua-goldstein Timely! I was struggling getting the testing (tox) working this week. Is there a plan to move to a more simple setup (basic unittest for instance)?

Yes, finally things are in working order now... With this PR tests should work locally. I just migrated to use pytest for now. We still need a script to do the cluster setup and tear down, so bash scripts/local-test.sh will do the cluster setup and then run the test suite. I think most of the tests we have here are integration tests.

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

Successfully merging this pull request may close these issues.

3 participants