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

build: moved to pyproject.toml/poetry #22

Closed
wants to merge 0 commits into from

Conversation

ErikBjare
Copy link
Collaborator

@ErikBjare ErikBjare commented Nov 8, 2020

Fixes #19, and more.

Changes

  • setup.py/requirements.txt replaced with pyproject.toml/poetry.lock
    • Dependencies are now managed/locked using poetry
  • pip install -r requirements.txt is now simply pip install .
    • Updated the documentation to reflect this fact.
    • Note that installing with pip install . will not use the exact versions in poetry.lock (but one can achieve the same thing by running poetry export -f requirements.txt; pip install -r requirements.txt as usual). Version constraints specified in pyproject.toml will still be respected.
  • Added basic GitHub Actions CI. (merged in ci: added basic ci #24)
    • It installs the package (with poetry) and then runs: mypy for typechecking, pytest for testing (but there are currently no tests to run).
  • Loosened version constraints for larger dependencies (which are unlikely to break backwards compatibility without DeprecationWarning)
  • Fixes ability to install directly from git with pip install git+https://...

TODO (if requested)

  • Build docs in CI (merged in ci: added basic ci #24)
  • Deal with the other requirements*.txt files, include them in pyproject.toml as dev dependencies or extras (that can be enabled with a flag)

I haven't done any extensive testing, but all seems to work as expected.

@JohnGriffiths
Copy link
Collaborator

Hi Erik.

This great - thanks!

Will do some review and testing very soon.

In the meantime, a few other comments:

  • The installation instructions do need changing to pip install -e . from pip install -r requirements.txt anyway, because that is needed for the new CLI command line program. And it's just the correct way to do it anyways. So, this is good that you have done that.

  • Would this also work with a pip install git+https://github.com/neurotechx/eeg-notebooks ? We would like to have that working so that an explicit git clone isn't necessary. I tried this once and it failed; not sure why.

  • ...and eventually (soon) we would like to put on pypi so it's just pip install

  • Yes, CI building for the docs is something we want to do also. The sphinx library generation is quite long working from scratch. Don't know if that is an issue.

  • TBH I'm thinking of removing the other requirements files. The extra doc building libraries are a minimal addition to the other large primary libraries. The no-eeg version could be useful in principle, but currently at least it isn't something we are using extensively. So, don't worry about those for now; they might disappear.

@ErikBjare
Copy link
Collaborator Author

ErikBjare commented Nov 8, 2020

Would this also work with a pip install git+https://github.com/neurotechx/eeg-notebooks? We would like to have that working so that an explicit git clone isn't necessary. I tried this once and it failed; not sure why.

Yes, just tested (but with https://github.com/ErikBjare/eeg-notebooks).

Edit: And reproduced the original issue in #19 (comment), looks like it's just a missing __init__.py.

...and eventually (soon) we would like to put on pypi so it's just pip install

Poetry will make that a lot easier for you :)

Yes, CI building for the docs is something we want to do also.

I'll take a look at it then, should be pretty easy.

Edit: Done.

@ErikBjare
Copy link
Collaborator Author

@JadinTredup Should be ready for review now. The CI can be inspected at https://github.com/ErikBjare/eeg-notebooks/actions (it should work automatically in this repo too, once merged).

@ErikBjare ErikBjare mentioned this pull request Nov 17, 2020
@ErikBjare
Copy link
Collaborator Author

ErikBjare commented Nov 17, 2020

I've extracted the CI stuff to a seperate PR #24, I'd appreciate a quick merge, so I can rebase this branch on top of upstream master.

@ErikBjare ErikBjare changed the title build: moved to pyproject.toml/poetry, added basic CI build: moved to pyproject.toml/poetry Nov 18, 2020
@ErikBjare
Copy link
Collaborator Author

ErikBjare commented Nov 18, 2020

PR has been rebased on master following the merge of #24.

Edit: Looks like the Windows build decided to start failing in this branch (see https://github.com/ErikBjare/eeg-notebooks/runs/1417206759?check_suite_focus=true) since it complains about missing Visual C++ build tools (which I thought should be included?). Weird.

@ErikBjare
Copy link
Collaborator Author

ErikBjare commented Jun 22, 2022

Trying to salvage this PR, but bumping into ridiculous dependency resolution times (caused by psychopy's ridiculous number of deps):

image

So will take at least 50min. Hoping it finishes soon...

I commented out psychopy to test, and it resolved in under 20min (iirc).

@ErikBjare
Copy link
Collaborator Author

Still not resolved, eating RAM and CPU like crazy:

image

@ErikBjare
Copy link
Collaborator Author

ErikBjare commented Jun 22, 2022

lol, the OOM killer ended up killing it:

image

dmesg:

image

Not sure what to do about this, I guess ask in python-poetry/poetry#2094 or bring it up with the psychopy devs if they can loosen their dep requirements? (or whatever is taking forever)

I guess I could try it again on my workstation with 64GB RAM + 64GB swap + 1Gbps? But would it even work?

Edit: Running again now with poetry update -vvv, not sure why I didn't do this sooner,

@ErikBjare
Copy link
Collaborator Author

ErikBjare commented Jun 22, 2022

Alright, I finally got a lock after ~1100 seconds on a different run, after I locked down some deps that I resolved by hand.

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.

Dependency versions are overly strict
2 participants