spaCy icon indicating copy to clipboard operation
spaCy copied to clipboard

CLI overrides in projects cause conflict with unset optional positional arguments

Open ines opened this issue 2 years ago • 3 comments

How to reproduce the behaviour

The project templates allow overriding variables via the CLI as --vars.foo, which uses the same mechanism we use for CLI config overrides. Under the hood, it works by setting the following on the typer CLI decorator, which will capture any additional arguments. We then parse those extra values using a helper function that interprets them and maps them to key/value pairs.

context_settings={"allow_extra_args": True, "ignore_unknown_options": True}

In the project CLI, this causes a problem with optional positional arguments – in this case, the project_dir, which defaults to the current working directory. If no project dir is provided and CLI overrides are set, the first override is interpreted as the positional argument. To work around this, the project directory has to be specified explicitly as ..

Example

vars:
  foo: bar

commands:
  - name: test
    script:
      - echo ${vars.foo}

This raises an error:

python -m spacy project run test --vars.foo baz
Error: Invalid value for '[PROJECT_DIR]': Directory '--vars.foo' does not exist.

This works as expected:

python -m spacy project run test . --vars.foo baz

Since this all happens at the level of typer when the arguments are provided (and before anything even gets to our helper functions), we can't easily solve this within our helpers. It's also pretty reasonable behaviour on typer/click's part IMO, since the logic for allowing extra args is really only supposed to capture extras (and it's us who use this feature in a more abstract way for additional arguments).

Making the project_dir required isn't really an option, because this would break the otherwise very convenient workflows. However, we could just accept this edge case and add a note to the docs that you need the . if you use CLI overrides.

I can't immediately think of an elegant solution, though, if we actually wanted to fix this in the code. We could remove the exists check from the project_dir arg, make it accept a string and then check whether whatever the CLI received as that positional argument startswith("--"). If so, we know it must be a CLI variable override and can't be the actual project directory, so we set project_dir to Path.cwd() and add the value starting with -- back to the overrides. This is a bit hacky, though, and we'll lose the automated CLI-level check that verifies that the project directory exists.

ines avatar Jul 28 '21 01:07 ines

While it's kind of unfortunate, for now I think I'd be in favour of adding a note to the docs, and perhaps also trying to catch when this happens and adding a custom error message? Because I don't really see an elegant solution otherwise either :|

svlandeg avatar Aug 02 '21 18:08 svlandeg

I think just documenting this is fine, since if someone is using arguments like this they're digging pretty deep into the system and can accept a wrinkle.

As another option, could we modify the input string before it gets to Typer? I'm not super familiar with how Typer works, but it looks like we can look for spacy project run [foo] --[bar]and rewrite the string if we wanted to. That would also be an ugly hack but it wouldn't mess with anything happening inside Typer at least.

(I just checked and while it's a terrible idea, sys.argv is writable...)

polm avatar Aug 12 '21 11:08 polm

@mborsetti: I deleted an unrelated comment about typer that you had posted in multiple places and that we have addressed elsewhere, to ensure this discussion stays on track.

svlandeg avatar Oct 06 '21 15:10 svlandeg