Is spacy.cli.download.download public API?

[iliakur]

Context

At work we wrap spacy and stanza in one codebase that provides uniform setup/access to models from both libraries. Part of this uniform access is downloading the models.
We're currently downloading Stanza models from inside Python and we'd like to do the same for spacy models. We're aware that this isn't exactly recommended by the docs, but we choose this option knowingly due to some other constraints.

Question

Can we import and invoke spacy.cli.download.download to perform the download? Afaict it's essentially a wrapper around pip install while taking care of some book-keeping. It also appears pretty stable based on commit history.

[adrianeboyd]

The spacy.cli.download.download API should be stable. I'm sure we're not quite at 100% on this, but any internal methods that don't start with _ should be stable even if they're not in the official API docs, and the CLI methods are quite stable in practice.

For this purpose, the main difference between stanza models and spacy models is that spacy models are separate python packages rather than just downloaded directories. Detecting which packages are currently installed in a python environment is expensive, so it's done once when a python script starts and if you install a model package in a running python process, it won't be found by spacy.load immediately afterwards. But if your download script is a separate script, this should fine. (Or you'll have to look for workarounds that refresh the internal state about loaded packages, which is possible, but enough of a headache, especially way in the past for python 2, that we haven't tried to support it internally.)

[iliakur]

Downloading and installing won't happen in the same process. However, we have a different wrinkle: we are mounting the models into a docker container. Which means we need to a) tell pip to install into a custom directory, b) tell spacy.load to load from the same directory. I guess b) can be achieved by tracking the full path to the downloaded model and passing that to spacy.load.

[iliakur]

Understood, thanks! I'll report here with my findings once we get it to work.

[iliakur]

Hmm, seems like I've stumbled across a bug. pip_args are completely ignored. This is because they are declared as variadic positional arguments after keyword arguments. Some examples to illustrate what I mean:

download(model, direct=True, sdist=False, "--no-deps") # -> `pip_args` is empty
download(model, "--no-deps", direct=True, sdist=False) # -> Error: multiple values for `direct`
download(model, "--no-deps") # -> `direct="--no-deps"

The fix for this was very easy, I just moved *pip_args before direct and sdist in the function declaration. I've noticed, however, that that corner of the code is somewhat under-tested. Getting it under test would require either some refactoring or willingness to perform a model download during testing (possible performance hit).

How do you prefer that we proceed? Should I just open a PR with the basic fix and follow up with some tests later, or should I also do the tests/refactoring?

[adrianeboyd]

Hmm, I'm sure this works from the command-line. Ah, I think it will work if you specify direct and sdist as args instead of kwargs, so:

download(model, True, False, "--no-deps")

This is definitely a bit weird, but I'd have to think a bit about the best way to modify this without breaking anything.

We do have some tests in the CI for the CLI commands that we don't have directly the unit tests because of the model downloads, which we restrict to one CI instance (just by python version, it's mildly hacky but works to test the compatibility table and a few other things):

spaCy/.github/azure-steps.yml

Lines 67 to 106 in 800737b

	- script: |
	python -m spacy download ca_core_news_sm
	python -m spacy download ca_core_news_md
	python -c "import spacy; nlp=spacy.load('ca_core_news_sm'); doc=nlp('test')"
	displayName: 'Test download CLI'
	condition: eq(variables['python_version'], '3.8')
	- script: |
	python -m spacy convert extra/example_data/ner_example_data/ner-token-per-line-conll2003.json .
	displayName: 'Test convert CLI'
	condition: eq(variables['python_version'], '3.8')
	- script: |
	python -m spacy init config -p ner -l ca ner.cfg
	python -m spacy debug config ner.cfg --paths.train ner-token-per-line-conll2003.spacy --paths.dev ner-token-per-line-conll2003.spacy
	displayName: 'Test debug config CLI'
	condition: eq(variables['python_version'], '3.8')
	- script: |
	# will have errors due to sparse data, check for summary in output
	python -m spacy debug data ner.cfg --paths.train ner-token-per-line-conll2003.spacy --paths.dev ner-token-per-line-conll2003.spacy | grep -q Summary
	displayName: 'Test debug data CLI'
	condition: eq(variables['python_version'], '3.8')
	- script: |
	python -m spacy train ner.cfg --paths.train ner-token-per-line-conll2003.spacy --paths.dev ner-token-per-line-conll2003.spacy --training.max_steps 10 --gpu-id -1
	displayName: 'Test train CLI'
	condition: eq(variables['python_version'], '3.8')
	- script: |
	python -c "import spacy; config = spacy.util.load_config('ner.cfg'); config['components']['ner'] = {'source': 'ca_core_news_sm'}; config.to_disk('ner_source_sm.cfg')"
	PYTHONWARNINGS="error,ignore::DeprecationWarning" python -m spacy assemble ner_source_sm.cfg output_dir
	displayName: 'Test assemble CLI'
	condition: eq(variables['python_version'], '3.8')
	- script: |
	python -c "import spacy; config = spacy.util.load_config('ner.cfg'); config['components']['ner'] = {'source': 'ca_core_news_md'}; config.to_disk('ner_source_md.cfg')"
	python -m spacy assemble ner_source_md.cfg output_dir 2>&1 | grep -q W113
	displayName: 'Test assemble CLI vectors warning'
	condition: eq(variables['python_version'], '3.8')

[iliakur]

Ok, let me know. The refactoring that occurred to me would take advantage of the run_command utility function. We don't have to test the download itself because that's essentially delegated to pip. We could just test that we generate the right pip command.

So download becomes (with args simplified for the sketch):

def download(*args, **kwargs):
    run_command(make_download_cmd(*args, **kwargs))

The main logic is then inside make_download_cmd which we can easily test because it only returns a sequence of strings, doesn't actually hit the network in any way.

The current behavior is definitely not intuitive and encourages opaque invocations of the function.

[iliakur]

I can confirm that passing direct and sdist as positional arguments does indeed work. Thanks for that :)

[iliakur]

@adrianeboyd just chiming in to let you know that modifying sys.path before calling spacy.load works fine!