Fix Readme and add some additional make commands #91
Conversation
|
@fhenneke Here is the PR. Also looks like there is no ci/cd. Not sure if you all are trying to do that but we could open up another ticket for! Please leave comments if you need to, thanks! |
There was a problem hiding this comment.
Hey @pierceroberts,
thanks for taking a stab at fixing the README. It really helps to have other people try out the code and propose instructions which would have helped them get it running.
The main issue I still have with the PR is the proposed use of pyenv. I cannot judge the benefits of this tool compared to things like uv, or if we should move to something more feature rich like poetry.
The fastest way to get the PR merged would be to remove the part on handling the python version. Most other comments should be straight forward to address.
If you want to push for a different tool to managing python (and/or package dependencies), you would have to provide more argument and also convince @harisang.
| To run the unittests you can use the make target unittest `make unittest`, however you might have a couple issues: | ||
| - You might run into the issue of the binary package for psycopg not being installed simply run: | ||
| ```sh | ||
| pip install "psycopg[binary,pool]" | ||
| ``` |
There was a problem hiding this comment.
Did this fail for you in a fresh environment? Then we should add it explicitly to requirements.in. Good catch!
Do we need only the binary or both binary and pool?
There was a problem hiding this comment.
It did not fail on pip install but it failed when I went to run the tests.
Personally I have only used psycopg2 because it's the newer version so I would advocate for that.
I haven't used psycopg3, but that is the newer version of psycopg2 as well. So maybe we could consider that?
here are the docs:
https://pypi.org/project/psycopg2/
I also had a question as to how you all are handling migrations are you using alembic or something?
| @@ -2,6 +2,11 @@ DOCKER_IMAGE_NAME=test_db_image | |||
| DOCKER_CONTAINER_NAME=test_db_container | |||
| DB_PORT=5432 | |||
|
|
|||
| # Might be worth using poetry here so you can install packages in the venv and then | |||
There was a problem hiding this comment.
Do you have a preference between poetry, uv, hatch, pyenv, or our approach with venv + pip + pip-tools?
I would like to keep external tools a minimum for now, and consistent with https://github.com/cowprotocol/solver-rewards and https://github.com/cowprotocol/ebbo.
We have discussed moving to some other tool but could not agree on one with good trade off of ease of use and added robustness.
There was a problem hiding this comment.
So as I mentioned in my other reply, pyenv is just for versioning. Poetry is for package management which replaces requirements.txt and creates a poetry.toml and poetry.lock file.
I am unsure on uv or hatch so I'll have to read into those.
I'd say requirements.txt does the job, but poetry is probably an upgrade.
| @@ -20,4 +25,7 @@ stop_test_db: | |||
| docker rm $(DOCKER_CONTAINER_NAME) || true | |||
| docker rmi $(DOCKER_IMAGE_NAME) || true | |||
|
|
|||
| unittest: | |||
| pytest tests/unit | |||
|
|
|||
| .PHONY: install imbalances daemon test_db run_test_db stop_test_db clean | |||
There was a problem hiding this comment.
Those new commands should probably also be added here, right?
There was a problem hiding this comment.
We can, but basically what the .PHONY represents is a way for make to determine what are make commands and what are files. If you have a file named install then make would be confused. If your targets are unique, you can avoid this.
Here is a stackoverflow post to help understand.
https://stackoverflow.com/questions/2145590/what-is-the-purpose-of-phony-in-a-makefile
|
Thanks for the feedback @fhenneke ! |
in reference to this issue.
#89