CLI - tuple parameters generate errors when in last position (before data type specification)

[MvWouden]

Describe the bug When tweaking model parameters using the command line, I found that tuple parameters throw errors depending on their position in the command. It seems that tuple parameters in the last position (before the data-type specification) do not work and throw errors. Reordering the parameters so the tuple is not in the last position resolves the issue.

To Reproduce

Example 1, eval-image-indices as tuple parameter, and a random non-tuple (int) parameter:

# Works
ns-train nerfacto ... --pipeline.datamanager.eval-image-indices 5 10 --pipeline.model.far-plane 10 nerfstudio-data ...

# Throws error
ns-train nerfacto ... --pipeline.model.far-plane 10 --pipeline.datamanager.eval-image-indices 5 10 nerfstudio-data ...

Example 2, num_proposal_samples_per_ray as tuple parameter, and a random (bool) non-tuple parameter:

# Works
ns-train nerfacto ... --pipeline.model.num_proposal_samples_per_ray 1024 512 --save-only-latest-checkpoint True nerfstudio-data ...

# Throws error
ns-train nerfacto ... --save-only-latest-checkpoint True --pipeline.model.num_proposal_samples_per_ray 1024 512 nerfstudio-data ...

Expected behavior

Having a tuple-parameter before the data type specification should work.

Additional context

I'm not sure whether this is an issue with tyro or with nerfstudio, so I posted it here.

The error generated is:

ns-train nerfacto: error: argument ...: invalid choice: 'none' (choose from 'nerfstudio-data', 'minimal-parser', 'arkit-data', 'blender-data', 'instant-ngp-data', 'nuscenes-data', 'dnerf-data', 'phototourism-data', 'dycheck-data', 'scannet-data', 'sdfstudio-data', 'nerfosr-data', 'sitcoms3d-data')

[tancik]

@brentyi I assume this is a Tyro issue?

[brentyi]

Yeah, the underlying issue here is in argparse.

Here's a minimal reproduction. If we specify a variable length argument (in this case --sequence), we have no way of specifying a position argument (in this case a or b) right after:

# sequences.py

import argparse

parser = argparse.ArgumentParser()
parser.add_argument("--sequence", nargs="+")
subparsers = parser.add_subparsers()
subparsers.add_parser("a")
subparsers.add_parser("b")

print(parser.parse_args())

The root cause here is that if we write python hello.py --sequence a b a b, all inputs will be considered part of the sequence.

This issue is pretty low-level, so I unfortunately don't have a good fix. Workarounds include: changing the annotation to a fixed-length sequence (annotate num_proposal_samples_per_ray as Tuple[int, int] instead of Tuple[int, ...]), but then we wouldn't be able to change the number of proposal networks, or using a string annotation that's parsed later instead "1024,512", but this feels hacky.

[brentyi]

Actually, a nicer fix would be tyro.conf.ConsolidateSubcommandArgs, which would change the syntax to ns-train nerfacto nerfstudio-data {--args for both nerfacto and nerfstudio-data}.

There would, however, be some obvious backward-compatibility downsides here.

[MvWouden]

Actually, a nicer fix would be tyro.conf.ConsolidateSubcommandArgs, which would change the syntax to ns-train nerfacto nerfstudio-data {--args for both nerfacto and nerfstudio-data}.

There would, however, be some obvious backward-compatibility downsides here.

Would it maybe be an option for tuples with numerical or boolean types to stop when it reaches the first non-numerical/non-boolean argument (or the end)? Obviously this wouldn't work for strings, but for booleans or numerical types this may work. I'm not sure I fully like this solution, but it would resolve this issue for non-string tuples.

[brentyi]

In theory yes! But it may be involved.

The stopping condition here is implemented by argparse, so the rule you described should reduce to modifying the snippet I shared earlier:

# sequences.py

import argparse

parser = argparse.ArgumentParser()
parser.add_argument("--sequence", nargs="+", type=int)
subparsers = parser.add_subparsers()
subparsers.add_parser("a")
subparsers.add_parser("b")

print(parser.parse_args())

If we're able to add the condition here in a clean-enough way, I can look into merging it into tyro. (and thus nerfstudio)