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

Use console_scripts entry points #4695

Merged
merged 1 commit into from Jul 14, 2020

Conversation

jwodder
Copy link
Member

@jwodder jwodder commented Jul 12, 2020

In #1384, it was decided to declare datalad's command-line commands via setup()'s scripts keyword instead of via entry_points. The reasoning given was that the entry point wrappers that setuptools generated were too slow. However, this decision ignored a crucial fact: Packages are normally installed with pip install, which uses a different set of entry points wrappers (provided by the distlib package) than those used when installing with setuptools via python setup.py install. (Note that installing a package in editable mode with pip install -e also uses the setuptools wrappers, as pip currently delegates editable installations to python setup.py develop.)

The entry point wrappers created by pip (both the latest version (20.1.1) and the current version when #1384 was written (9.0.1)) look like:

#!/usr/bin/python3

# -*- coding: utf-8 -*-
import re
import sys

from datalad.cmdline.main import main

if __name__ == '__main__':
    sys.argv[0] = re.sub(r'(-script\.pyw?|\.exe)?$', '', sys.argv[0])
    sys.exit(main())

Compare this to the scripts that datalad currently generates:

#!/usr/bin/python3
#
# Custom simplistic runner for DataLad. Assumes datalad module
# being available.  Generated by monkey patching monkey patched
# setuptools.
#
from datalad.cmdline.main import main
main()

The biggest difference is a regex, which (based on my unscientific experiments) does not seem to have an appreciable effect on the command's runtime. Hence, there is no real downside to using entry_points as long as users are installing with pip install (which has been the recommended way to install Python packages for many years now).

As a benefit, switching back to always using entry_points instead of autogenerating scripts will allow the project to begin distributing wheels again, as there will be no difference in the arguments passed to setup() on Windows vs. Unix (cf. #4315).

@yarikoptic
Copy link
Member

Hi @jwodder, thank you very much for the PR and the detailed description! I would have loved to get rid of this hack, but:

If I didn't miss anything though in the description, getting back to always using entry_points would mean that pip install -e (preferable installation method for the development) installed datalad would get back to slow setuptools based entry points? Then it would negatively impact run time and unit tests and thus DX (developer experience) without giving a palpable benefit (besides wheels which give some benefit to user experience). Or am I missing something?

@jwodder
Copy link
Member Author

jwodder commented Jul 12, 2020

That is true, though I believe it's considered better practice not to use development mode when testing in the first place, as (for one thing) it can mask packaging problems[1]. Development mode should really only be necessary if you're running ad hoc commands & scripts with your package; running a test suite should be done through tox or the like, which doesn't care whether you install in development or non-development mode (though none of the developers seem to have used tox with this project recently, as the tox.ini tries to test under Python 2.7 even though the project is only for 3.5 and up).

[1] For example, if you fail to properly declare a package or data file, the file will still be there when testing in development mode and your tests will succeed, whereas installing in non-development mode means you're testing against what your users get, and so a missing resource would cause tests to fail. It may also be necessary to use a src/ layout in addition to eschewing development mode in order to get this benefit; I'm not sure.

Side note: Ever since version 47.3.0 of setuptools was released, the setuptools entry points wrappers have been much faster if you're running under Python 3.8 or have importlib_metadata installed.

@yarikoptic
Copy link
Member

yarikoptic commented Jul 13, 2020

Development mode is really convenient for development - I can change code and rerun command or test without needing (to remember) to reinstall. We run tests on Travis in installed (not development) mode, so it picks up any rare cases where something was left behind.
Since datalad tests battery takes too long, and CIs come with their own python environments, tox kinda lost its value for us, at least for now.

@mih
Copy link
Member

mih commented Jul 13, 2020

Thx @jwodder !

My 2ct: I am very much in favor of merging this PR. The number of milliseconds saved in a development scenario on Linux do not hold up against the number of hours I sunk into debugging installation issues on windows, only to eventually realize that two completely different methods are being used.

@yarikoptic why do you think that the unittest runtime would be impacted? There are of course a few tests that actually exercise the main script, but those should be minuscule in comparison to the total runtime.

@yarikoptic
Copy link
Member

Entry points are also custom special remotes. I guess it is just a matter of doing some timing and see (I could be wrong and may be impact is tiny).

@yarikoptic
Copy link
Member

But also recent #4690 came to mind, which shows about 200 starts of special remote while testing crawler

@yarikoptic
Copy link
Member

yarikoptic commented Jul 13, 2020

some initial timing

with current maint (0.13.0-66-gd40822340)
$> timeit datalad --help
10 runs real:0.83+-0.02 user:0.72+-0.03 sys:0.10+-0.02
timeit datalad --help  7.66s user 1.18s system 100% cpu 8.800 total

$> time python -m nose datalad/customremotes
............S.S
----------------------------------------------------------------------
Ran 15 tests in 36.891s
and with this PR
$> timeit datalad --help
10 runs real:0.95+-0.04 user:0.85+-0.04 sys:0.09+-0.01
timeit datalad --help  8.98s user 1.18s system 99% cpu 10.263 total

$> time python -m nose datalad/customremotes
............S.S
----------------------------------------------------------------------
Ran 15 tests in 45.505s

so about 100msec added on datalad entrypoint startup, which (I guess) results in ~10 sec (>25% time) added to testing datalad/customremotes. This is done with python 3.8.3rc1 on debian, with setup tools still being 44.0.0-1 . And Debian still has only 46.1.3-1 in unstable ATM.

`pip upgrade` ing setuptools seems to result in some speed up indeed for individual entry point but somehow tests still take old time to run:
$> timeit datalad --help                    
10 runs real:0.89+-0.02 user:0.79+-0.01 sys:0.10+-0.01

$> time python -m nose datalad/customremotes
............S.S
----------------------------------------------------------------------
Ran 15 tests in 46.259s
and apparently I had importlib_metadata 1.5.0 installed:
$> pip install importlib_metadata
Requirement already satisfied: importlib_metadata in /usr/lib/python3/dist-packages (1.5.0)
upgrading it to 1.7.0 had detrimental effect on `datalad --help` but helped the customremotes tests!
$> timeit datalad --help
10 runs real:1.12+-0.23 user:0.97+-0.19 sys:0.14+-0.04

$> time python -m nose datalad/customremotes
............S.S
----------------------------------------------------------------------
Ran 15 tests in 37.797s

with similar negative impact on "faster" datalad install --help (0.44 vs 0.70 sec, so almost twice slower).

Since all those aren't really "important", I will meanwhile run and time a full sweep of tests in maint and this PR to see overall effect.

minor side discovery: it seems that the order of entries in our help output is not "stable", here is the diff between maint and this branch:
$> diff -Naur /tmp/1 /tmp/2
--- /tmp/1	2020-07-13 11:02:23.628920375 -0400
+++ /tmp/2	2020-07-13 11:02:15.392753125 -0400
@@ -86,12 +86,12 @@
       Export the content of a dataset as a TAR/ZIP archive
   export-to-figshare
       Export the content of a dataset as a ZIP archive to figshare
-  addurls
-      Create and update a dataset from a list of URLs
   no-annex
       Configure a dataset to never put some content into the dataset's annex
   check-dates
       Find repository dates that are more recent than a reference date
+  addurls
+      Create and update a dataset from a list of URLs
 
 *Plumbing commands*

@yarikoptic
Copy link
Member

FWIW, the sweep across tests (first by mistake on maint, then master, then this branch):

$> grep 'Ran .*tests' tests-sweep-0.13.0-*
tests-sweep-0.13.0-36-g547acd948.log:Ran 1166 tests in 4112.111s
tests-sweep-0.13.0-65-ge61a8c26f.log:Ran 1171 tests in 4709.775s
tests-sweep-0.13.0-66-gd40822340.log:Ran 1171 tests in 3938.225s

suggests that it is quite ok (although I think the middle one, master, was a bit compromised by me also working on the laptop at that point but I expect it to be only a bit longer than the first one anyways).

So, given that I am already developing on 3.8 and we could for devel installation request a fresh importlib_metadata -- impact would not be that bad (individual commands invocation slow down remains a mystery to me at this point).

@yarikoptic
Copy link
Member

On our weekly call we agreed that we should proceed forward with this. Thanks @jwodder !

@yarikoptic
Copy link
Member

Note: not bringing to maint

@yarikoptic yarikoptic merged commit eda6c9e into datalad:master Jul 14, 2020
@jwodder jwodder deleted the use-console-scripts branch November 21, 2020 03:00
@jwodder jwodder mentioned this pull request May 17, 2021
8 tasks
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.

None yet

3 participants