Fix numpy annotations for pybind v3.0.0

[sarlinpe]

• Copied tests from v2.13 and updated for the changes introduced by v3.0.0
  • int -> typing.SupportsInt and similarly for float
  • set/list/dict and generic types in typing -> collections.abc equivalent
• Unified the parsing of annotation strings in FixNumpyArrayDimTypeVar and FixNumpyArrayDimAnnotation to handle both old-style (pre-v3.0.0) and new-style annotations.
  • old-style: np.ndarray[PRIMITIVE_TYPE[*DIMS], *FLAGS]
  • new-style:
    typing.Annotated[numpy.typing.ArrayLike, PRIMITIVE_TYPE, "[*DIMS]", "*FLAGS"]
typing.Annotated[numpy.typing.NDArray[PRIMITIVE_TYPE], "[*DIMS]", "*FLAGS"]
typing.Annotated[numpy.typing.ndarray[Any, PRIMITIVE_TYPE], "[*DIMS]", "*FLAGS"]
typing.Annotated[typing.Annotated[NDArray[...]], "[m, n]", "*FLAGS"]
• Handle again the deprecated numpy.bool dtype since v3.0.0 apparently uses it (instead of the native bool)
• Fix a bug resulting in parse_annotation_str not being called on the first nested level of a union, thereby breaking the parsing of Union[Annotated[numpy...], None]
• Check test errors with --numpy-array-wrap-with-annotated since v3.0.0 does not anymore throw errors on missing import for dynamic shape variables.

Unrelated to v3.0.0 but necessary to run tests locally: ensure the tests are built with the correct C++ standard

These fixes have been working well for us at https://github.com/colmap/colmap

Fixes #264
Fixes #266
Fixes #272

[gentlegiantJGC]

I would be interested in merging this into my fork here.
Would you consider opening a pull request there?
I have already added tests for pybind 3 so it should make the diff a bit easier to understand.

[sarlinpe]

Given the massive amount of work that this has already required, I would rather merge this here.

@sizmailov would you mind having a look when you find the time? Thank you!

[gentlegiantJGC]

I tried reviewing your changes but the addition of pybind11 v3 stubs made it difficult to see what had changed.
If this was merged as a separate pull request it would make reviewing the actual changes easier.

I added pybind11 v3 tests to my fork before realising you had also implemented them here.
#271 is group of pull requests from my fork including v3 tests and some dependent pull requests.
If merged first it would simplify this pull request quite a bit.

[dyollb]

This is a great effort! Here are some comments:

Problem 1: Deeply Nested Conditional Logic in _from_new_style

@classmethod
def _from_new_style(cls, resolved_type: ResolvedType) -> Optional[_NumpyArrayAnnotation]:
    if resolved_type.parameters is None or len(resolved_type.parameters) == 0:
        return None

    # Handle nested Annotated: Annotated[Annotated[NDArray[...]], "[m, n]"]
    if (
        len(resolved_type.parameters) > 1
        and isinstance(resolved_type.parameters[0], ResolvedType)
        and resolved_type.parameters[0].name in cls.__typing_annotated_names
    ):
        # Nested logic...
    else:
        # More logic...

    # Then multiple elif branches for different array types...
    if array_type.name == QualifiedName.from_str("numpy.typing.ArrayLike"):
        # ArrayLike specific logic
    elif array_type.name == QualifiedName.from_str("numpy.typing.NDArray"):
        # NDArray specific logic 
    elif array_type.name == QualifiedName.from_str("numpy.ndarray"):
        # ndarray specific logic
    else:
        return None

Maybe you can break this is into smaller, focused methods?

@classmethod
def _from_new_style(cls, resolved_type: ResolvedType) -> Optional[_NumpyArrayAnnotation]:
    if not cls._validate_new_style_input(resolved_type):
        return None
    
    array_type_param, other_params = cls._extract_annotation_params(resolved_type)
    if not cls._validate_array_type(array_type_param):
        return None
        
    return cls._build_from_array_type(array_type_param, other_params)

@classmethod
def _extract_annotation_params(cls, resolved_type: ResolvedType):
    """Extract array type and other parameters, handling nested annotations."""
    # Handle nested annotations separately
    
@classmethod 
def _build_from_array_type(cls, array_type_param, other_params):
    """Build annotation based on specific array type."""
    # Delegate to type-specific handlers

Problem 2: Inconsistent Error Handling Patterns

The code has multiple locations returning None without logging or context. Maybe you could return a AnnotationParseResult, e.g.

@dataclass
class AnnotationParseResult:
    annotation: Optional[_NumpyArrayAnnotation]
    error: Optional[str] = None
    
@classmethod
def _from_new_style(cls, resolved_type: ResolvedType) -> AnnotationParseResult:
    if resolved_type.parameters is None:
        return AnnotationParseResult(None, "No parameters in Annotated type")

Problem 3: Hard-coded Dimension Pattern __dim_string_pattern = re.compile(r'"\[(.*?)\]"')

• assumes dimensions are always in double quotes "[...]", what if single quotes or no quotes?
• the .*? non-greedy match could fail with nested structures, e.g., "[matrix[3, 4], vector[2]]"
• the regex matches any content between brackets without validating it's actually dimension data

Problem 4: Loss of Context in def _to_dims(cls, dimensions: Sequence[ResolvedType | Value | InvalidExpression]) -> list[int | str] | None:

• again, maybe return error messages, not just None when parsing fails

Problem 5: Large PR

• this is why I asked copilot to help me review this MR 😉

[chpatrick]

Hi @dyollb, are those all blockers? It would be nice to have this so we can package pycolmap for NixOS. :) @sizmailov ?

[dyollb]

Hi @dyollb, are those all blockers? It would be nice to have this so we can package pycolmap for NixOS. :) @sizmailov ?

I don't think so but I am not a maintainer. We also need this for str | os.PathLike annotations (currently the os module is not imported)

[dyollb]

We have been using this patch for a while in a very large codebase. It fixes our issues with union type hints, e.g. for filesystem path support. I hope it is merged into master soon.

[chpatrick]

@sizmailov Any chance we could merge this? I would like to package pycolmap on NixOS.

[ax3l]

Hi @skarndev , do you like to help review this PR and bring it to merge? :)

[sarlinpe]

Feel free to take it apart into smaller PRs, I'm unfortunately out of time to do this myself... Thank you!

[ax3l]

Thank you, besides the tests/stub changes, the PR mostly focuses on a 400 LOC change in pybind11_stubgen/parser/mixins/parser.py.

@sarlinpe are you ok if we (e.g., @skarndev) push to this PR as maintainers? (The option is on, which is great.)

[sarlinpe]

Of course, please go ahead!