-
Notifications
You must be signed in to change notification settings - Fork 5
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
Use virtual environments for development #181
base: master
Are you sure you want to change the base?
Conversation
Instead of trying to support all possible platforms via an `install.sh` script (which would need `root` privileges, in most cases), we should just do a better job of documenting our system requirements.
We had this almost working, but, after `pip install`'ing our package, it was crashing with $ ultratrace Traceback (most recent call last): File "/home/kevinmurphy/src/SwatPhonLab/UltraTrace/venv/ultratrace/bin/ultratrace", line 33, in <module> sys.exit(load_entry_point('ultratrace', 'console_scripts', 'ultratrace')()) File "/home/kevinmurphy/src/SwatPhonLab/UltraTrace/venv/ultratrace/bin/ultratrace", line 25, in importlib_load_entry_point return next(matches).load() File "/home/kevinmurphy/src/SwatPhonLab/UltraTrace/venv/ultratrace/lib/python3.8/site-packages/importlib_metadata/__init__.py", line 169, in load return functools.reduce(getattr, attrs, module) AttributeError: module 'ultratrace.__main__' has no attribute 'main'
Right now, this is basically unsupported. If we actually want to build out some tools for managing test-data, we should probably write something a bit more robust.
Instead of rewriting from scratch, it'll be a lot easier to just tweak the existing system...
The `setup.cfg` should just declare "abstract" requirements (which libraries do we need at runtime?). The "concrete" requirements (which version of each package do we need?) should be declared in the `requirements.txt`. See also: https://packaging.python.org/en/latest/discussions/install-requires-vs-requirements/ See also: https://caremad.io/posts/2013/07/setup-vs-requirement/
Weirdly, these need to go into different files... * https://mypy.readthedocs.io/en/stable/config_file.html#the-mypy-configuration-file * https://flake8.pycqa.org/en/latest/user/configuration.html#configuration-locations
2. Make sure you have pip | ||
3. Install ffmpeg and add to PATH | ||
4. Run `setup.py` | ||
TODO |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what else to say here.. I don't know how to install system packages on Windows. Maybe we should remove this section altogether? Once the system packages are installed, everything else should be the same...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not, because of non-Python library dependencies.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
non-Python library dependencies.
Yeah this is what I mean by "system packages".
We already directly depend on this package!
cc @jonorthwash -- not sure if you want to take a look / give feedback or if you'd rather I just merge. |
This was just from running $ black ultratrace/
AFAICT, we didn't actually need this for anything... Closes #176
The Phonetics Lab currently has three machines that run Debian (not to mention my office computer and personal machines). Could we make sure that's explicitly supported? |
|
||
To lint/test `ultratrace`, use [`nox`](https://nox.thea.codes/en/stable/): | ||
```sh | ||
$ nox --help |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't say how to do anything specific.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, maybe it would be better to just say to run nox
directly?
$ nox
nox > Running session install
nox > Creating virtual environment (virtualenv) using python in .nox/install
nox > python -m pip install --requirement ./requirements.txt
nox > Session install was successful.
There's more info if you follow that link to their docs, but basically running plain nox
will run all of the tests defined here: https://github.com/SwatPhonLab/UltraTrace/blob/feature/install-into-virtual-environment/noxfile.py
black==22.10.0 | ||
mypy==0.982 | ||
nox==2022.8.7 | ||
pytest==7.1.3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've never used most of these before. Why are they being introduced as dependencies?
pydub==0.25.1 | ||
pyparsing==3.0.9 | ||
python-dateutil==2.8.2 | ||
python-magic==0.4.27 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will break on Windows and probably macOS, and I wouldn't be surprised if locking it down to a specific version breaks on Linux in the future as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why would pinning versions break? I think we're more likely to see breakages with unpinned versions..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because not the same versions are available on all OSes of the packages that rely on compiled C libraries.
Can we separate out some of these into different PRs? Like removing the ultratrace2 stuff I would merge right away, and I'd like to evaluate some of the other changes independently of one another. This is a bit much in one PR for me... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See comments throughout.
Autoformat Python code
@keggsmurph21, I've tested this PR and it seems to work okay. But the conflicts need to be resolved before I can merge it. |
This PR cleans up a bunch of stuff, including
dev/env.sh
script (see the README)ultratrace2
cruftHopefully, this should make it easier to hack on this project now that everything can happen inside a virtual environment.
Unfortunately, I still can't help much with the folks working on Windows machines...