Opened 15 months ago

Closed 2 months ago

Last modified 2 months ago

#30586 closed defect (fixed)

requirements are not included in setup.py

Reported by: irl Owned by: phw
Priority: Medium Milestone:
Component: Metrics/Onionperf Version:
Severity: Normal Keywords: metrics-team-roadmap-2020
Cc: acute, metrics-team Actual Points: 1.15
Parent ID: #33321 Points: 0.5
Reviewer: Sponsor: Sponsor59-must

Description

The requirements are not listed in setup.py which complicates installation a bit. The requirements in requirements.txt are not in sync with the actual project requirements.

Child Tickets

Attachments (1)

0001-Adds-setuptools-and-alternative-installation-instruc.patch (2.1 KB) - added by acute 2 months ago.

Download all attachments as: .zip

Change History (22)

comment:1 Changed 12 months ago by irl

Resolution: invalid
Status: newclosed

comment:2 Changed 6 months ago by acute

Resolution: invalid
Status: closedreopened

Moving all gitlab OP tickets back to Trac.

comment:3 Changed 3 months ago by gaba

Keywords: metrics-team-roadmap-2020 added
Points: 0.5
Sponsor: Sponsor59

comment:4 Changed 3 months ago by karsten

Sponsor: Sponsor59Sponsor59-must

Moving to Sponsor59-must, because we should really do these in order to call Sponsor59 done.

comment:5 Changed 3 months ago by phw

Owner: changed from metrics-team to phw
Status: reopenedassigned

comment:6 Changed 3 months ago by phw

Status: assignedneeds_review

I tried to find undocumented modules by running:

find onionperf/ -name "*.py" | xargs grep -h import | sort | uniq

This only yielded nose and nose-cov, which are used for tests.

I also tried to find unused modules by running:

grep --exclude-dir=venv -ri MODULE

This yielded Twisted, cycler, decorator, pyparsing, python-dateutil, pytz, six, and zope.interface.

Here's my patch set: https://github.com/NullHypothesis/onionperf/compare/defect/30586

comment:7 Changed 3 months ago by acute

I've pushed a minor fixup commit onto my github branch to remove the duplication of the requirements:

https://github.com/ana-cc/onionperf/commit/893306c97bff0526c42ee97dda789ebf310304be.patch

With this applied, it is good to merge. Phw, if you are happy with this you can set this to merge-ready!

comment:8 in reply to:  7 Changed 3 months ago by phw

Actual Points: 0.25
Status: needs_reviewmerge_ready

Replying to acute:

I've pushed a minor fixup commit onto my github branch to remove the duplication of the requirements:

https://github.com/ana-cc/onionperf/commit/893306c97bff0526c42ee97dda789ebf310304be.patch

With this applied, it is good to merge. Phw, if you are happy with this you can set this to merge-ready!


That's a useful patch, thanks! I force-pushed it to my GitHub branch, making this ready for merge now.

comment:9 Changed 3 months ago by acute

Actual Points: 0.250.35
Cc: metrics-team added

Adding my 0.1 points to yours, and CC'ing metrics-team!

comment:10 Changed 3 months ago by karsten

Thanks for these patches! And thanks for copying metrics-team; I had not seen the earlier updates (oh, Trac...).

So, I tried out this branch without installing all dependencies via sudo apt install python3-*, and it starts downloading packages via pip. In our Java projects we have always avoided using package managers and only relied on packages provided by the system. We would give up this principle here, but are we sure we want to do that? How are we doing this in other Python projects at Tor?

comment:11 Changed 3 months ago by karsten

Parent ID: #33321

comment:12 Changed 3 months ago by karsten

With #33258 being merged we'll also have to add python3-pandas and python3-seaborn to the list of requirements.

comment:13 in reply to:  10 ; Changed 3 months ago by acute

Replying to karsten:

Thanks for these patches! And thanks for copying metrics-team; I had not seen the earlier updates (oh, Trac...).

So, I tried out this branch without installing all dependencies via sudo apt install python3-*, and it starts downloading packages via pip. In our Java projects we have always avoided using package managers and only relied on packages provided by the system. We would give up this principle here, but are we sure we want to do that? How are we doing this in other Python projects at Tor?

I was wondering whether there is a distinction here that we need to make - how do {researchers, developers} install and use onionperf on their machines vs how we deploy onionperf in Tor.

For general purpose use/development, using python-setuptools (which is a standard way of packaging python modules and uses pip to manage dependencies) and then running Onionperf on the command line is entirely reasonable, and we could leave this documented as is in the deployment instructions.

For 'official' deployments, pip is not ideal. There, it makes more to install dependencies directly from Debian, and use a systemd service or a tmux session (this would be harder to monitor) to run Onionperf.
The actual 'python3 setup.py install' step is not even necessary, as you can just run onionperf directly from the git repository, as long as the PYTHONPATH is set.
An alternative would be to install and run it in a virtual environment, using the system packages (venv --system-site-packages) for dependencies instead of fetching them with pip.

This is similar to what the ansible workflow does/did in metrics-cloud.

In the long term, once the codebase matures it would be a good idea to package this in Debian - it looks like other teams that use pip for deployment are also looking to move to Debian packages.

Last edited 3 months ago by acute (previous) (diff)

comment:14 Changed 3 months ago by acute

Actual Points: 0.350.45

comment:15 in reply to:  13 Changed 3 months ago by phw

Replying to acute:

Replying to karsten:

Thanks for these patches! And thanks for copying metrics-team; I had not seen the earlier updates (oh, Trac...).

So, I tried out this branch without installing all dependencies via sudo apt install python3-*, and it starts downloading packages via pip. In our Java projects we have always avoided using package managers and only relied on packages provided by the system. We would give up this principle here, but are we sure we want to do that? How are we doing this in other Python projects at Tor?

I was wondering whether there is a distinction here that we need to make - how do {researchers, developers} install and use onionperf on their machines vs how we deploy onionperf in Tor.


That's a good point. For third parties, installation should be quick, easy, and work the way it does for other Python tools. For ourselves, we do whatever makes the most sense for us – and that doesn't have to be documented in the README.md.

On an unrelated note: I added a commit to my patch set that adds the pandas and seaborn requirement: https://github.com/NullHypothesis/onionperf/compare/defect/30586

comment:16 Changed 3 months ago by karsten

Okay, I agree with the general approach to have different steps for researchers and developers and for ourselves.

However, I'm still having difficulties with building and running OnionPerf without using pip: The command python3 setup.py install --user still runs pip in the background, and I'm unclear how to set PYTHONPATH in order to avoid running that command in the first place. Once I know how to do that I can merge this branch (and apply the #33433 patch on top of it).

Another minor issue is that python3-setuptools needs to be listed as required package in the instructions now. I can add that to README.md, if that's the only remaining issue.

Thanks for your patience! :)

comment:17 in reply to:  16 Changed 2 months ago by acute

Replying to karsten:

However, I'm still having difficulties with building and running OnionPerf without using pip: The command python3 setup.py install --user still runs pip in the background, and I'm unclear how to set PYTHONPATH in order to avoid running that command in the first place. Once I know how to do that I can merge this branch (and apply the #33433 patch on top of it).

Apologies, I should have added this.
Once all the dependencies are installed for Onionperf, this is as simple as:

git clone https://git.torproject.org/onionperf.git
cd onionperf/onionperf
PYTHONPATH=.. python3 onionperf

Another minor issue is that python3-setuptools needs to be listed as required package in the instructions now. I can add that to README.md, if that's the only remaining issue.

I've attached a patch for README.md that includes python3-setuptools in the instructions, and also includes the instructions above on how to run OP without installing it - should be a quick review :)

Thanks for your patience! :)

Thanks very much, sorry for taking longer than anticipated.

comment:18 Changed 2 months ago by karsten

Almost there! Two further questions:

  1. What if I want to run onionperf from another directory than onionperf/onionperf? I tried passing another relative or absolute path to PYTHONPATH=, but all I get is: /usr/bin/python3: can't find '__main__' module in 'onionperf'.
  1. Do you mind posting a new branch with phw's branch rebased to current master and with your two patches (the one on #33433 and the one above) on top? I would do this myself, but I'm having trouble applying your patches, and I don't want to claim authorship of these commits.

Thanks!

comment:19 in reply to:  18 Changed 2 months ago by acute

Replying to karsten:

Almost there! Two further questions:

  1. What if I want to run onionperf from another directory than onionperf/onionperf? I tried passing another relative or absolute path to PYTHONPATH=, but all I get is: /usr/bin/python3: can't find '__main__' module in 'onionperf'.

You also need to specify the absolute/relative path to the onionperf script as well in that case, for example:

PYTHONPATH=/home/dev/onionperf python3 /home/dev/onionperf/onionperf/onionperf

Just as a note, you can copy the script itself anywhere else on the system and this will still work, as long as you point the PYTHONPATH to the onionperf directory.

cp /home/dev/onionperf/onionperf/onionperf /home/dev/op
PYTHONPATH=/home/dev/onionperf python3 /home/dev/op

or even directly:

PYTHONPATH=/home/dev/onionperf /home/dev/op

...since the python3 interpreter is in a sensible place anyway.
Hope this helps!

  1. Do you mind posting a new branch with phw's branch rebased to current master and with your two patches (the one on #33433 and the one above) on top? I would do this myself, but I'm having trouble applying your patches, and I don't want to claim authorship of these commits.

This is now here:
https://github.com/ana-cc/onionperf/tree/acute/30586

Thank you!

comment:20 Changed 2 months ago by karsten

Actual Points: 0.450.85
Resolution: fixed
Status: merge_readyclosed

Thanks a lot for the detailed explanation of how PYTHONPATH works. I had somehow assumed that the onionperf script would be found by specifying the OnionPerf directory in PYTHONPATH. When specifying both paths it works as expected.

Thanks, also, for rebasing and providing all patches as a single path. That's quite a bit easier for me to review and merge.

Pushed to master. Finally! Closing.

comment:21 Changed 2 months ago by acute

Actual Points: 0.851.15

Adding my 0.3 actual points to yours :)

Note: See TracTickets for help on using tickets.