Improve type checking by haudren-woven · Pull Request #679 · ros2/launch

haudren-woven · 2022-12-01T01:27:18Z

This is still a work in progress, but following @clalancette 's comments on #676 , I made this PR to:

Introduce the mypy type checker, and check how long it takes to run (On my machine a couple of seconds)
Show some examples of fixes that are necessary for some files to pass

Note that we will probably need to tweak the mypy configuration a little bit, since currently non-typed imports are treated as errors.

haudren-woven · 2022-12-01T01:34:12Z

The test indeed failed, and the run time is indeed around 1 or 2 seconds:

10:30:23 test/test_flake8.py .                                                    [  1%]
10:30:31 test/test_mypy.py F                                                      [  2%]
10:30:32 test/test_pep257.py .                                                    [  2%]

methylDragon · 2022-12-01T19:26:44Z

If the test is failing, could you set this PR to draft until you have a working version?

Edit: I'm not sure how possible it would be to introduce a test that would fail on merge... It looks like whatever's causing the type errors to happen need to be fixed alongside this PR

haudren-woven · 2022-12-02T00:41:00Z

@methylDragon : Sorry about that, I should have made it draft. I just wanted to address @clalancette 's concerns quickly. I will convert to draft, and try to fix the type issues, please hold on 🙏

- Declare the type of a dictionary - Add missing return - Annoyingly the return type of get_entity is a union of all possible types. Ideally, get_entity would return the same type as what's passed in by allowed_data_types (and possibly string) but that would require some advanced type wrangling to achieve template-like functionality. For now, use casts where necessary.

- Touchup process_targeted_events and shutdown_process to accept a ExecuteLocal instead of ExecuteProcess - Still an issue surrounding OnExit - Many minor fixes

Just change the input type

Mostly casts around get attribute

haudren-woven · 2023-01-06T03:24:06Z

@methylDragon : First of all sorry for the long delay. I got really busy in work stuff, then went on holidays. I'm resuming work on this, and one sticky method has been the Entity.get_attr. Its behavior (with regards to typing) is a little hard to capture:

data_type in the original signature can be only one of AllowedTypesType, which is pretty much either a scalar or a list of scalars. However, in the code it's very frequently used with a List[Entity]. Could you maybe shed some light on what is the real type bound on the data types of get_attr?
can_be_str is not documented in the abstract class. I understand well that in the case of a scalar, it means that the return type would be a Union[Scalar, str], but what about a list? Can only the individual elements of the list be left as string (e.g. List[Union[Scalar, str]]) or is it the whole list (e.g. Union[List[Scalar], str]).
In terms of interaction between can_be_str and optional, I'd like to make sure that my understanding is correct: when both are set to True, is it fair to say that we will return None if the attribute is absent, a string if the attribute is present but cannot be coerced to the correct type, the type otherwise?

Thank you!

The overloads allow us to properly distinguish between: - Data type passed or not passed (returns str) - Optional set to true or false (returns Optional[T] or T) Also remove the now redundant casts.

- Event handlers - Boolean substitutions

haudren-woven · 2023-01-06T06:53:49Z

Another few questions:

Surrounding OnProcessExit: in that file, a number of cast are performed to basically perform an upcast from functions taking the narrow type ExecuteLocal into more general functions taking the abstract Action type. I don't quite understand how this can work since OnActionEventBase seems to call the action matcher callable with the action itself, without checking if the action that generated the event is indeed a subclass of target_action_cls.
For the OpaqueCoroutine, there seems to be a great deal of confusion between a coroutine and a coroutine function. As far as I can tell, the documentation (and the type annotations) seem to think this foo object is a coroutine:
```
    async def foo() -> None:
       pass
```
However, in this case foo is a coroutine function, e.g. a Callable which returns a Coroutine. I can fixup the type annotations fairly easily, but that issue is present in a few places, so I'd like to hear everyone's opinion on the best course here.
I was quite surprised to see this code in some_actions_type:
```
SomeActionsType = Union[
    LaunchDescriptionEntity,
    Iterable[LaunchDescriptionEntity],
]
```
This causes issues in some places where the return type is still Iterable[Action], which is incompatible with Iterable[LaunchDescriptionEntity]. I guess this is the result of some refactoring, but how should we address the situation: keep the name SomeActionsType or rename it to SomeEntitiesType?

haudren-woven · 2023-01-11T23:47:51Z

@adityapande-1995 @methylDragon @wjwwood @clalancette : any comments / remarks on the above points?

haudren-woven · 2023-02-15T00:20:39Z

@adityapande-1995 @methylDragon @wjwwood @clalancette : Sorry to bother everyone, but could someone take a quick look at the above remarks? Then I can resume work on this PR

methylDragon · 2023-02-15T02:03:45Z

Hey @haudren-woven , sorry for dropping this, and thanks again for all the hard work!

Unfortunately, I don't think I have the necessary context/experience with the launch frontend to give definitive answers for this, but I can give it a shot... (though I think @wjwwood would have more of the necessary context)

Data Type

data_type in the original signature can be only one of AllowedTypesType, which is pretty much either a scalar or a list of scalars. However, in the code it's very frequently used with a List[Entity]. Could you maybe shed some light on what is the real type bound on the data types of get_attr?

I think in this case, let's expand the list of allowable types to what's actually being used in source.

Consider the usage of data_type for get_attrs for the keep attribute in this xml example (since keep attribute access is one of the cases where the entity returns a list of entities):

        <launch>
            <let name="foo" value="FOO"/>
            <let name="bar" value="BAR"/>
            <reset>
                <keep name="bar" value="$(var bar)"/>
                <keep name="baz" value="BAZ"/>
            </reset>
        </launch>

If either bar or baz were not strings, then to get all the keep children of the reset entity, the get_attrs method would have to be able to return a heterogeneous list of children.

So in this case I think it would be reasonable to allow for List[Entity] to also be a valid data_type.
Let's then set data_type as Union[AllowedTypesType, List[Entity]], I think.

can_be_str

can_be_str is not documented in the abstract class. I understand well that in the case of a scalar, it means that the return type would be a Union[Scalar, str], but what about a list? Can only the individual elements of the list be left as string (e.g. List[Union[Scalar, str]]) or is it the whole list (e.g. Union[List[Scalar], str]).

Consider the other available docstrings for can_be_str in the other methods:

In the case a value can be either an instance of a type or a substitution, the can_be_str argument of get_attr must be used
:param can_be_str: when True, non-uniform lists mixed with strings are allowed.

I think in no cases do we want to convert a single list entity into a string in its entirety. So I think we want List[Union[Scalar, str]].

The reason for this is to think about how the Entities are expected to be used (parsed entities to be manipulated programatically). If we convert a list entity into a string, then it'd require a re-parse, which I don't think is what the intended use is. I might be wrong about this though.

Also: Ideally we'd end up documenting the can_be_str attribute for the abstract class, but I'm not sure if my understanding is correct. I'd defer to @wjwwood for this, but in case this needs to be merged now, I think it'd be wise to add a note/todo, while adding the current understanding of the usage. (Where the Entity can be either a string or a scalar, in the case of a list of entities, a heterogeneous list of scalars/entities AND/OR strings.)

optional

In terms of interaction between can_be_str and optional, I'd like to make sure that my understanding is correct: when both are set to True, is it fair to say that we will return None if the attribute is absent, a string if the attribute is present but cannot be coerced to the correct type, the type otherwise?

I think this is correct.

I will respond to the later post soon.

methylDragon · 2023-02-15T02:36:06Z

OnProcessExit

Surrounding OnProcessExit: in that file, a number of cast are performed to basically perform an upcast from functions taking the narrow type ExecuteLocal into more general functions taking the abstract Action type. I don't quite understand how this can work since OnActionEventBase seems to call the action matcher callable with the action itself, without checking if the action that generated the event is indeed a subclass of target_action_cls.

That... sounds like a subtle type bug. I'm not sure what way to best resolve it.

Coroutines

For the OpaqueCoroutine, there seems to be a great deal of confusion between a coroutine and a coroutine function. As far as I can tell, the documentation (and the type annotations) seem to think this foo object is a coroutine:
    async def foo() -> None:
       pass
However, in this case foo is a coroutine function, e.g. a Callable which returns a Coroutine. I can fixup the type annotations fairly easily, but that issue is present in a few places, so I'd like to hear everyone's opinion on the best course here.

Again, I'm not sure I have all the context, but I'd be in favor of fixing the docs/annotations to match how each function is currently being used (especially if it's a widespread pattern.)

some_actions_type

I was quite surprised to see this code in some_actions_type:
SomeActionsType = Union[
    LaunchDescriptionEntity,
    Iterable[LaunchDescriptionEntity],
]
This causes issues in some places where the return type is still Iterable[Action], which is incompatible with Iterable[LaunchDescriptionEntity]. I guess this is the result of some refactoring, but how should we address the situation: keep the name SomeActionsType or rename it to SomeEntitiesType?

I would be in favor of a rename (and a file move as well.)

haudren-woven · 2023-02-16T00:34:45Z

Thank you very much for the thoughtful answers! I will proceed with these, but it might take a little while... Please hold on 🙏

methylDragon · 2023-02-16T03:15:09Z

Thank you very much for the thoughtful answers! I will proceed with these, but it might take a little while... Please hold on 🙏

Heyyy no worries about this! Take your time!
Feel free to ping me again if I don't do timely replies, and I'll help best I can. Though I'm not holding all of the context

Do not change public API just yet

- Fixup some quotes and line lengths - Many orders needed to be reordered

haudren-woven · 2023-02-28T04:21:47Z

@methylDragon 32fa4cc should have fixed the CI errors :)

methylDragon · 2023-02-28T06:47:22Z

Ok got it! I need to install python3-flake8-import-order, which is somehow not a rosdep? Anyhow, that's great :)

:O

No it's there as a rosdep, maybe you have to update your rosdep

haudren-woven · 2023-02-28T07:24:45Z

I meant it doesn't seem to be a key inside the package.xml so rosdep install --from-paths src doesn't seem to pick it up 🤔 Not sure how it's installed in CI 🤔

methylDragon · 2023-02-28T08:44:51Z

I meant it doesn't seem to be a key inside the package.xml so rosdep install --from-paths src doesn't seem to pick it up 🤔 Not sure how it's installed in CI 🤔

OH! I think you're meant to call ament_flake8 instead of running flake8 manually :>

methylDragon · 2023-02-28T08:45:22Z

Linux
Linux-aarch64
Windows

methylDragon · 2023-02-28T18:49:03Z

Just three more lint errors!

haudren-woven · 2023-03-01T00:12:25Z

Oh no... I'm sorry I really need to learn how to run the linters locally...

haudren-woven · 2023-03-01T00:12:49Z

Oh no... I'm sorry I really need to learn how to run the linters locally...

haudren-woven · 2023-03-01T00:18:59Z

Should be better now :)

methylDragon · 2023-03-01T00:48:45Z

🤞

Linux
Linux-aarch64
Windows

haudren-woven · 2023-03-01T01:24:50Z

Seems to have failed due to random connection dropping...

09:55:12 Caused: hudson.remoting.ChannelClosedException: Channel "hudson.remoting.Channel@50a4fcb8:JNLP4-connect connection from ip-10-0-1-61.ec2.internal/10.0.1.61:46536": Remote call on JNLP4-connect connection from ip-10-0-1-61.ec2.internal/10.0.1.61:46536 failed. The channel is closing down or has closed down

methylDragon · 2023-03-01T11:41:54Z

Aarch:

I'm not sure why mypy is finding errors on windows: https://ci.ros2.org/job/ci_windows/18774/testReport/junit/launch.test/test_mypy/test_mypy/

haudren-woven · 2023-03-01T23:46:33Z

I see what's going on:

12:16:04 test\launch\utilities\test_signal_management.py:55:14: error: Module has no attribute "SIGUSR1"
12:16:04 test\launch\utilities\test_signal_management.py:56:22: error: Module has no attribute "SIGUSR2"

This means that these statements are protected by a guard that mypy does not recognize...

This check is recognized by mypy properly!

haudren-woven · 2023-03-02T01:09:17Z

@methylDragon : Seems to have passed on my machine using mypy --platform win32. Now time to 🙏

I also noticed that ament_mypy doesn't allow to pass arbitrary mypy command-line flags, I'm wondering if it's something we should add...

methylDragon · 2023-03-02T19:17:35Z

Windows:

haudren-woven · 2023-03-02T23:46:57Z

Hum two test failures on Windows, but they don't seem super related 🤔 I'll have a closer look but are any flaky tests known?

methylDragon · 2023-03-03T03:31:23Z

No harm in trying again

methylDragon · 2023-03-03T18:47:04Z

Okay, I'm just gonna say that they're unrelated flaky tests that are failing, since its a different set of test failures now 😬

Merging!! Thanks for the incredible work! 🚀

emersonknapp · 2025-08-28T04:44:25Z

@Mergifyio backport humble

mergify · 2025-08-28T04:44:34Z

backport humble

✅ Backports have been created

Details

#905 Improve type checking (backport #679) has been created for branch humble but encountered conflicts

(cherry picked from commit 06dc66b) # Conflicts: # launch/launch/logging/__init__.py # launch/launch/substitutions/launch_log_dir.py # launch/launch/substitutions/python_expression.py # launch/launch/substitutions/this_launch_file.py # launch/launch/substitutions/this_launch_file_dir.py

haudren-woven added 2 commits December 1, 2022 10:24

Add test for type checking

53828e7

Fix some type errors

51f3aab

haudren-woven requested review from adityapande-1995, methylDragon and wjwwood as code owners December 1, 2022 01:27

haudren-woven marked this pull request as draft December 2, 2022 00:41

haudren-woven added 5 commits January 5, 2023 17:05

Fix declare launch argument

430f594

Fixup execute_local

9d3f77a

- Touchup process_targeted_events and shutdown_process to accept a ExecuteLocal instead of ExecuteProcess - Still an issue surrounding OnExit - Many minor fixes

Fixup path substitution

2beeb74

Just change the input type

Fixup execute process

550f034

Mostly casts around get attribute

Use overloads to make get_attr type safe

9f070d4

The overloads allow us to properly distinguish between: - Data type passed or not passed (returns str) - Optional set to true or false (returns Optional[T] or T) Also remove the now redundant casts.

haudren-woven force-pushed the feat/type-checking branch from 1110881 to 9f070d4 Compare January 6, 2023 03:30

haudren-woven added 2 commits January 6, 2023 15:50

Minor fixes in include launch description

63af13f

More small fixes

f588667

- Event handlers - Boolean substitutions

Fixup minor errors in launch config

239858e

haudren-woven added 3 commits February 16, 2023 18:59

Rename some_actions_type -> some_entities_type

a8446cb

Rename coroutine -> coroutine_function

e4b8edc

Do not change public API just yet

Minor typing mistake

dfb1653

Make flake8 happy!

32fa4cc

- Fixup some quotes and line lengths - Many orders needed to be reordered

One more round of lint...

4b17097

Use sys.platform to check for Windows

a2e87bb

This check is recognized by mypy properly!

methylDragon merged commit 06dc66b into ros2:rolling Mar 3, 2023

This was referenced Mar 6, 2023

Remove the import of Literal from entity.py. #694

Merged

Use SomeEntitiesType for type checking. ros2/launch_ros#358

Merged

This was referenced Apr 16, 2025

Local execute action fails to flush process output on exit #859

Closed

Fix ExecuteLocal output flushing #860

Merged

mergify bot mentioned this pull request Aug 28, 2025

Improve type checking (backport #679) #905

Open

Conversation

haudren-woven commented Dec 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

haudren-woven commented Dec 1, 2022

Uh oh!

methylDragon commented Dec 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

haudren-woven commented Dec 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

haudren-woven commented Jan 6, 2023

Uh oh!

haudren-woven commented Jan 6, 2023

Uh oh!

haudren-woven commented Jan 11, 2023

Uh oh!

haudren-woven commented Feb 15, 2023

Uh oh!

methylDragon commented Feb 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Data Type

can_be_str

optional

Uh oh!

methylDragon commented Feb 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

OnProcessExit

Coroutines

some_actions_type

Uh oh!

haudren-woven commented Feb 16, 2023

Uh oh!

methylDragon commented Feb 16, 2023

Uh oh!

haudren-woven commented Feb 28, 2023

Uh oh!

methylDragon commented Feb 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

haudren-woven commented Feb 28, 2023

Uh oh!

methylDragon commented Feb 28, 2023

Uh oh!

methylDragon commented Feb 28, 2023

Uh oh!

methylDragon commented Feb 28, 2023

Uh oh!

haudren-woven commented Mar 1, 2023

Uh oh!

haudren-woven commented Mar 1, 2023

Uh oh!

haudren-woven commented Mar 1, 2023

Uh oh!

methylDragon commented Mar 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

haudren-woven commented Mar 1, 2023

Uh oh!

methylDragon commented Mar 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

haudren-woven commented Mar 1, 2023

Uh oh!

haudren-woven commented Mar 2, 2023

Uh oh!

methylDragon commented Mar 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

haudren-woven commented Mar 2, 2023

Uh oh!

methylDragon commented Mar 3, 2023

Uh oh!

methylDragon commented Mar 3, 2023

Uh oh!

emersonknapp commented Aug 28, 2025

Uh oh!

mergify bot commented Aug 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

haudren-woven commented Dec 1, 2022 •

edited

Loading

methylDragon commented Dec 1, 2022 •

edited

Loading

haudren-woven commented Dec 2, 2022 •

edited

Loading

methylDragon commented Feb 15, 2023 •

edited

Loading

methylDragon commented Feb 15, 2023 •

edited

Loading

methylDragon commented Feb 28, 2023 •

edited

Loading

methylDragon commented Mar 1, 2023 •

edited

Loading

methylDragon commented Mar 1, 2023 •

edited

Loading

methylDragon commented Mar 2, 2023 •

edited

Loading

mergify bot commented Aug 28, 2025 •

edited

Loading