Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add frontend module in launch, launch_xml and launch_yaml packages #226

Merged
merged 75 commits into from Jul 11, 2019

Conversation

@ivanpauno
Copy link
Contributor

ivanpauno commented Apr 15, 2019

See ros2/design#207 and ros2/design#208.
Targets all the tasks of #163 (comment) (except the last three).

This is already functional.

TODOs (in importance order):

  • Write launch_yaml readme ✔️
  • Write more action and substitution parsing functions. ✔️
  • Write more xml and yaml examples to exercise the written code. ✔️
  • launch_frontend package should probably be merged with launch, as they are completely coupled.
    Action and Substitution parsing methods should be moved to the classes (see
    ros2/launch_ros#23) (nice to have). ✔️
  • Create a TestEntity to unit test the parsing methods (nice to have).
  • See if we can write a decorator that infers the parsing function from the constructor typing annotations.
    That is a really tricky tasks (nice to have).

P.S.:
launch_frontend/launch_frontend/parse_substitution.py (later modified by me) and launch_frontend/launch_frontend/grammar.lark were contributed by @hidmic.

Depends on #235 (after 3c1c1e9) (already merged).

@ivanpauno ivanpauno added the in review label Apr 15, 2019
@ivanpauno ivanpauno requested a review from hidmic Apr 15, 2019
@ivanpauno ivanpauno self-assigned this Apr 15, 2019
@hidmic hidmic mentioned this pull request Apr 17, 2019
Copy link
Contributor

hidmic left a comment

@ivanpauno alright, looking good, thanks for pushing this!

launch_frontend/launch_frontend/parser.py Outdated Show resolved Hide resolved
launch_frontend/package.xml Outdated Show resolved Hide resolved
launch_frontend/setup.py Outdated Show resolved Hide resolved
launch_xml/README.md Outdated Show resolved Hide resolved
launch_xml/package.xml Outdated Show resolved Hide resolved
launch_xml/setup.py Outdated Show resolved Hide resolved
launch_xml/test/launch_xml/executable.xml Outdated Show resolved Hide resolved
launch_xml/README.md Show resolved Hide resolved
launch_frontend/launch_frontend/parser.py Outdated Show resolved Hide resolved
launch_frontend/launch_frontend/parser.py Outdated Show resolved Hide resolved
@ivanpauno ivanpauno force-pushed the ivanpauno/launch-frontend branch from c46b260 to 0d2e6ac Apr 22, 2019
Copy link
Contributor

hidmic left a comment

Second pass, sorry for the delay to review :(

launch_frontend/launch_frontend/parser.py Outdated Show resolved Hide resolved
launch_xml/README.md Outdated Show resolved Hide resolved
launch_frontend/launch_frontend/parser.py Outdated Show resolved Hide resolved
@ivanpauno ivanpauno force-pushed the ivanpauno/launch-frontend branch from 992f531 to b28736f May 10, 2019
@ivanpauno ivanpauno changed the title Add launch_frontend and launch_xml Entity Add launch_frontend, launch_xml and launch_yaml May 10, 2019
@ivanpauno ivanpauno requested a review from hidmic May 10, 2019
Copy link
Contributor

hidmic left a comment

Wow, that's a large PR but I have to say it really honors the design document. It looks amazing, great job man!

launch_frontend/launch_frontend/entity.py Outdated Show resolved Hide resolved
launch_frontend/launch_frontend/expose.py Outdated Show resolved Hide resolved
launch_frontend/launch_frontend/parser.py Outdated Show resolved Hide resolved
```python
'int', 'float', 'bool', 'list[int]', 'list[float]', 'list[bool]', 'list[str]', 'str'
```

This comment has been minimized.

Copy link
@hidmic

hidmic May 15, 2019

Contributor

@ivanpauno meta: IMHO, type deduction documentation is design document material.

This comment has been minimized.

Copy link
@ivanpauno

ivanpauno May 17, 2019

Author Contributor

If you want, I can update ros2/design#208 and ros2/design#207, and create a new PR for yaml design doc.

This comment has been minimized.

Copy link
@hidmic

hidmic May 20, 2019

Contributor

Eventually, yeah, but let's hold that for a minute. At least until we've settled here.

launch_yaml/launch_yaml/entity.py Outdated Show resolved Hide resolved
launch_yaml/launch_yaml/entity.py Outdated Show resolved Hide resolved
launch_yaml/setup.py Outdated Show resolved Hide resolved
launch_yaml/test/launch_yaml/test_executable.py Outdated Show resolved Hide resolved
@hidmic

This comment has been minimized.

Copy link
Contributor

hidmic commented May 15, 2019

launch_frontend package should probably be merged with launch, as they are completely coupled.
Action and Substitution parsing methods should be moved to the classes (see
ros2/launch_ros#23) (nice to have).

I fully agree and also think we need to devise a way to reuse parsing code in derived actions and substitutions.

@hidmic

This comment has been minimized.

Copy link
Contributor

hidmic commented May 15, 2019

See if we can write a decorator that infers the parsing function from the constructor typing annotations.
That is a really tricky tasks (nice to have).

Yeah, that is tricky. I'll try to take a stab at it at some point, should be possible though.

@ivanpauno ivanpauno force-pushed the ivanpauno/launch-frontend branch from 400e070 to 2100794 May 17, 2019
def test_combining_quotes_nested_substitution():
subst = default_parse_substitution(
'$(env "asd_bsd_qsd_$(test \'asd_bds\')" \'$(env DEFAULT)_qsd\')'
)

This comment has been minimized.

Copy link
@ivanpauno

ivanpauno May 17, 2019

Author Contributor

@hidmic I added this new test case. For having a more uniform behavior, I think this should look like:
'$(env "asd_bsd_qsd_$(test \\\'asd_bds\\\')" \'$(env DEFAULT)_qsd\')'
(this is not a raw a string, I'm escaping both the \ and ')

For that, I would change the following in the grammar:

SINGLE_QUOTED_STRING: (/[^'\\"$]|\$(?=!\()|(?<=\\)\$/ | "\\\"" | "\\'" | "\\")+
SINGLE_QUOTED_RSTRING: (/[^ '\\"$\(\)]|(?<=\\)\(|(?<=\\)\)|\$(?=!\()|(?<=\\)\$/ | "\\\"" | "\\'" | "\\")+

DOUBLE_QUOTED_STRING: (/[^"\\'$]|\$(?=!\()|(?<=\\)\$/ | "\\\"" | "\\'" | "\\")+
DOUBLE_QUOTED_RSTRING: (/[^ "\\'$\(\)]|(?<=\\)\(|(?<=\\)\)|\$(?=!\()|(?<=\\)\$/ | "\\\"" | "\\'" | "\\")+

Basically, I'm stopping allowing any kind of quote inside SINGLE_QUOTED_* and DOUBLE_QUOTED_*, and only allowing it if it is escaped.

Do you agree with that change?

This comment has been minimized.

Copy link
@hidmic

hidmic May 20, 2019

Contributor

I'm fine with it, though it may be counter intuitive for some accustomed to combine quotes. If we do it, we should clearly document it somewhere. @wjwwood any thoughts on this i.e. on forcing quote escaping everywhere if one wants to keep it raw?

This comment has been minimized.

Copy link
@wjwwood

wjwwood Jun 27, 2019

Member

Escaping quotes inside of quotes is ok by me.

@ivanpauno ivanpauno force-pushed the ivanpauno/launch-frontend branch from 5e1a0b4 to 9c3a9f6 May 31, 2019
@ivanpauno

This comment has been minimized.

Copy link
Contributor Author

ivanpauno commented May 31, 2019

Rebased with master again.

@ivanpauno ivanpauno added this to Proposed in Dashing Patch Release 1 via automation Jun 4, 2019
@jacobperron jacobperron removed this from Proposed in Dashing Patch Release 1 Jun 6, 2019
@ivanpauno ivanpauno added this to In progress in Eloquent via automation Jun 6, 2019
Copy link
Contributor

hidmic left a comment

@ivanpauno I think we need to polish overall documentation format a bit too.

launch_frontend/launch_frontend/entity.py Outdated Show resolved Hide resolved
- 'list[Entity]'
'guess' work in the same way as:
('int', 'float', 'bool', 'list[int]', 'list[float]', 'list[bool]', 'list[str]', 'str')

This comment has been minimized.

Copy link
@hidmic

hidmic Jun 10, 2019

Contributor

@ivanpauno why not replace 'guess' by the absence of a type and make it the default?

This comment has been minimized.

Copy link
@ivanpauno

ivanpauno Jun 10, 2019

Author Contributor

Because the type that is specified most of the times is str (substitutions are str).

This comment has been minimized.

Copy link
@hidmic

hidmic Jun 10, 2019

Contributor

Same, making str the default.

launch_frontend/launch_frontend/entity.py Outdated Show resolved Hide resolved
it will check if the attribute read match with one in `types`.
If it is one of them, the value is returned.
If not, an `TypeError` is raised.
- For frontends that don't natively recognize data types (like xml),

This comment has been minimized.

Copy link
@hidmic

hidmic Jun 10, 2019

Contributor

@ivanpauno hmm, these two bullets are packed with assumptions about downstream implementations and markup languages that I'd prefer to spare. Consider just stating that the types argument states the expected types of a given attribute and whether that results in type coercion or type checking depends on the particular frontend.

This comment has been minimized.

Copy link
@hidmic

hidmic Jun 10, 2019

Contributor

Also, now that I think of it, it might make sense to let the user choose between checks and coercions.

return default_parse_substitution(value)

@classmethod
def parse_description(cls, entity: Entity) -> launch.LaunchDescription:

This comment has been minimized.

Copy link
@hidmic

hidmic Jun 10, 2019

Contributor

@ivanpauno hmm, I'm somewhat inclined to say that these methods don't need nor should be class methods.

This comment has been minimized.

Copy link
@ivanpauno

ivanpauno Jun 11, 2019

Author Contributor

parse_description, parse_action should. cls is finally passed to the action parsing methods as parser argument. The parsing methods could call parse.substitution, which can be overridden by the derived class.

This comment has been minimized.

Copy link
@hidmic

hidmic Jun 12, 2019

Contributor

Right, my point being that these should be instance methods.

This comment has been minimized.

Copy link
@ivanpauno

ivanpauno Jun 13, 2019

Author Contributor

The Parse instances don't have a state now, but it could have it in the future, so I think it is a good idea. I will change parse_action, parse_substitution, parse_description to instance methods.
load, load_parser_extensions and load_parser_implementations will be continue being class methods.
I will stop returning an object of Parse type in the load method (ie: a subclass of Parse) , and start returning an instance of it.
Note: I will also rename load_parser_extensions as load_launch_extensions.

launch_frontend/launch_frontend/parser.py Outdated Show resolved Hide resolved
@@ -0,0 +1,155 @@
# Copyright 2019 Open Source Robotics Foundation, Inc.

This comment has been minimized.

Copy link
@hidmic

hidmic Jun 10, 2019

Contributor

@ivanpauno after going over this file a couple times, I think that using strings for type identification is a bit clunky. We could use type objects as well, which would then simplify the implementation. Thoughts?

This comment has been minimized.

Copy link
@ivanpauno

ivanpauno Jun 11, 2019

Author Contributor

The problem is with 'list[int]', etc. Using typing objects doesn't help much in the implementation (i.e.: I don't have an easy way to extract if it was a list, and what is the item type).

This comment has been minimized.

Copy link
@hidmic

hidmic Jun 12, 2019

Contributor

Right, but you can check for equality just like you do with strings. And then you don't have to do substring checks.

This comment has been minimized.

Copy link
@hidmic

hidmic Jun 24, 2019

Contributor

I still wonder if it wouldn't be better to use typing objects instead of plain strings.

This comment has been minimized.

Copy link
@wjwwood

wjwwood Jun 27, 2019

Member

I don't have a strong opinion. Using typing would be cool, but I don't know if it makes it faster or cleaner.

This comment has been minimized.

Copy link
@hidmic

hidmic Jun 27, 2019

Contributor

So, between using typing classes and using a stringification of their names, I'm slightly inclined towards the first one. Collection type information is readily available and the API looks cleaner that way IMO. No need to do substring checks anymore.

launch_frontend/launch_frontend/parser.py Outdated Show resolved Hide resolved
"""Load parser extension, in order to get all the exposed substitutions and actions."""
if cls.extensions_loaded is False:
for entry_point in iter_entry_points('launch_frontend.parser_extension'):
entry_point.load()

This comment has been minimized.

Copy link
@hidmic

hidmic Jun 10, 2019

Contributor

@ivanpauno meta: we can leave it as is for a first iteration, but I'm not entirely convinced that working with importation side effects is in our best interest in the long run. I wonder if having some sort of index at each package root module that its classes can expose/register themselves into would be cleaner than a having a global one. Maybe @wjwwood can weigh in too on this.

This comment has been minimized.

Copy link
@wjwwood

wjwwood Jun 27, 2019

Member

Are you talking about using pkg_resources or something else (sorry I didn't really grok "importation side effects" in this context)?

pkg_resources is known to be a bit slow, but this should be a one time activity. Of course, we don't want launch startup to be slow either, so I don't have a strong feeling one way or the other. We talked about other solutions in catkin tools, and some guy actually commented that they made a replacement system:

catkin/catkin_tools#297

We could use ament index instead, but I don't have any strong feelings about it one way or the other.

This comment has been minimized.

Copy link
@hidmic

hidmic Jun 27, 2019

Contributor

It's not so much about performance (though I had not event considered that, thanks for the reference) but the way actions, substitutions and parsers are discovered. It's currently loading a global collection inside launch upon entry point loading.

I was wondering if it wouldn't make sense to move away from that and make exposure somewhat more explicit, and maybe introducing namespaces in the process (which we do not have, we just fail upon tag name collision). Anyways, just a thought. No need to address this now, really.

Eloquent automation moved this from In progress to Review in progress Jun 10, 2019
ivanpauno added 5 commits Apr 15, 2019
Signed-off-by: ivanpauno <ivanpauno@ekumenlabs.com>
Signed-off-by: ivanpauno <ivanpauno@ekumenlabs.com>
Signed-off-by: ivanpauno <ivanpauno@ekumenlabs.com>
Signed-off-by: ivanpauno <ivanpauno@ekumenlabs.com>
Signed-off-by: ivanpauno <ivanpauno@ekumenlabs.com>
@ivanpauno

This comment has been minimized.

Copy link
Contributor Author

ivanpauno commented Jul 8, 2019

Finally, CI (only fastrtps, up to launch launch_ros launch_yaml launch_xml test_launch_ros):

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status
ivanpauno added 3 commits Jul 8, 2019
Signed-off-by: ivanpauno <ivanpauno@ekumenlabs.com>
…ink-install

Signed-off-by: ivanpauno <ivanpauno@ekumenlabs.com>
Signed-off-by: ivanpauno <ivanpauno@ekumenlabs.com>
@ivanpauno

This comment has been minimized.

Copy link
Contributor Author

ivanpauno commented Jul 9, 2019

Again, now that CI problems have been solved:

  • Linux Build Status (aborted intentionally)
  • Linux-aarch64 Build Status
  • Windows Build Status (problems using issubclass(x, List))
Signed-off-by: ivanpauno <ivanpauno@ekumenlabs.com>
@ivanpauno

This comment has been minimized.

Copy link
Contributor Author

ivanpauno commented Jul 9, 2019

Checking if windows is happy after f15b0f2:

  • Windows Build Status

(Edit) New failures on windows, checking locally.

Signed-off-by: ivanpauno <ivanpauno@ekumenlabs.com>
ivanpauno added 4 commits Jul 9, 2019
Signed-off-by: ivanpauno <ivanpauno@ekumenlabs.com>
Signed-off-by: ivanpauno <ivanpauno@ekumenlabs.com>
Signed-off-by: ivanpauno <ivanpauno@ekumenlabs.com>
Signed-off-by: ivanpauno <ivanpauno@ekumenlabs.com>
@ivanpauno

This comment has been minimized.

Copy link
Contributor Author

ivanpauno commented Jul 9, 2019

CI ...

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status
@hidmic
hidmic approved these changes Jul 10, 2019
Copy link
Contributor

hidmic left a comment

LGTM but for a few minor comments. This is simply awesome, great job @ivanpauno !

launch/launch/frontend/entity.py Outdated Show resolved Hide resolved
launch/launch/frontend/entity.py Outdated Show resolved Hide resolved
launch/launch/frontend/type_utils.py Outdated Show resolved Hide resolved
launch/launch/frontend/type_utils.py Outdated Show resolved Hide resolved
launch/launch/frontend/type_utils.py Outdated Show resolved Hide resolved
launch/launch/frontend/type_utils.py Outdated Show resolved Hide resolved
launch/launch/frontend/type_utils.py Outdated Show resolved Hide resolved
launch/launch/frontend/type_utils.py Outdated Show resolved Hide resolved
keys = {
int: 0,
float: 1,
bool: 2,

This comment has been minimized.

Copy link
@hidmic

hidmic Jul 10, 2019

Contributor

@ivanpauno meta: I wonder if bool should come before int

This comment has been minimized.

Copy link
@ivanpauno

ivanpauno Jul 10, 2019

Author Contributor

No, if not 0 will be converted to bool. If both are possible, it should be an int.
And maybe we should just delete that option. So the rules do not overlap.

This comment has been minimized.

Copy link
@hidmic

hidmic Jul 11, 2019

Contributor

If both are possible, it should be an int.

Ok, fair enough, Python coercion will do its job.

launch_xml/README.md Outdated Show resolved Hide resolved
Eloquent automation moved this from Review in progress to Reviewer approved Jul 10, 2019
Signed-off-by: ivanpauno <ivanpauno@ekumenlabs.com>
@ivanpauno ivanpauno changed the title Add launch_frontend, launch_xml and launch_yaml Add frontend module in launch, launch_xml and launch_yaml packages Jul 11, 2019
@ivanpauno

This comment has been minimized.

Copy link
Contributor Author

ivanpauno commented Jul 11, 2019

@hidmic I've addressed all the comments in both PRs. I will run an extra CI after an approval.

@hidmic
hidmic approved these changes Jul 11, 2019
Copy link
Contributor

hidmic left a comment

LGTM!

@ivanpauno

This comment has been minimized.

Copy link
Contributor Author

ivanpauno commented Jul 11, 2019

Hopefully, last CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status
@ivanpauno ivanpauno merged commit 40c9d87 into master Jul 11, 2019
1 check passed
1 check passed
DCO DCO
Details
Eloquent automation moved this from Reviewer approved to Done Jul 11, 2019
@delete-merged-branch delete-merged-branch bot deleted the ivanpauno/launch-frontend branch Jul 11, 2019
@wjwwood wjwwood mentioned this pull request Jul 23, 2019
30 of 34 tasks complete
piraka9011 added a commit to aws-ros-dev/launch that referenced this pull request Aug 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Eloquent
  
Done
3 participants
You can’t perform that action at this time.