Closed ErikBjare closed 2 years ago
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.
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 https://github.com/NeuroTechX/eeg-notebooks/issues/19#issuecomment-724560996, 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.
@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).
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.
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.
Trying to salvage this PR, but bumping into ridiculous dependency resolution times (caused by psychopy's ridiculous number of deps):
So will take at least 50min. Hoping it finishes soon...
I commented out psychopy to test, and it resolved in under 20min (iirc).
Still not resolved, eating RAM and CPU like crazy:
lol, the OOM killer ended up killing it:
dmesg:
Not sure what to do about this, I guess ask in https://github.com/python-poetry/poetry/issues/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,
Alright, I finally got a lock after ~1100 seconds on a different run, after I locked down some deps that I resolved by hand.
Fixes #19, and more.
Changes
setup.py
/requirements.txt
replaced withpyproject.toml
/poetry.lock
poetry
pip install -r requirements.txt
is now simplypip install .
pip install .
will not use the exact versions inpoetry.lock
(but one can achieve the same thing by runningpoetry 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 #24)poetry
) and then runs: mypy for typechecking, pytest for testing (but there are currently no tests to run).DeprecationWarning
)pip install git+https://...
TODO (if requested)
Build docs in CI(merged in #24)requirements*.txt
files, include them inpyproject.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.