drake
drake copied to clipboard
RobotLocomotion
Parser.AddAllModelsFromFile('my_model_directives.dmd.yaml')
It would be very nice to be able to parse model directives using the same API as we use for other file formats.
As discussed in https://drakedevelopers.slack.com/archives/C2WBPQDB7/p1654939678154139, it might be nice to designate a more specific filename extension (e.g. .dmd.yaml instead of just .yaml).
@EricCousineau-TRI @rpoyner-tri @calderpg-tri would you like weigh in on what filename extension we should use for this?
(I'd like to move this forward sooner than later. A lot our our planning stack on the way to Drake would greatly benefit from the better sugar here.)
I would lean towards sticking with either of the common yaml extensions .yaml or .yml to get out-of-the-box editor highlighting for beginners. Multiple extensions like .dmd.yaml seems more likely to be confusing.
I agree with filename.endswith(".yaml") as an invariant, so that editors work easily and out of the box. I don't think using multiple extensions defeats that -- the editors I'm familiar with still treat it as YAML even with multiple extensions.
We do need some kind of additional convention to clarify the schema of the file.
In some cases, projects can omit the extra extension stuff because other clues lead to the choice of schema. For example, docker-compose.yml always has the same basename, so we know what its syntax is. In other cases, there might be a farm of yaml files all under a given conf folder, which all share a uniform schema.
In our case, however, the model directives might be named anything, and appear in any folder, so we do need some kind of policy for how to recognize them.
That could be with dual extensions like /path/to/foo.model_directives.yaml or with a required basename suffix like /path/to/foo_model_directives.yaml. I don't particularly care which naming policy we choose, but we do need something beyond **/*.yaml.
Having a naming convention will improve readability and reduce confusion, not the opposite.
I think having multiple extensions, e.g. *.dmd.yaml, is completely fine, and I'd personally prefer that over basename suffix. Ideally, whatever rule we choose for the Parser routine, we make it required for directives in isolation.
Re; the full feature, my only concern for this is the nuance when a model directives file isn't encapsulated, and thus can't be parsed in isolation (or, more generally, in the wrong context). Adding some text in a docstring or a tutorial would be helpful for when the questions inevitably come.
@rpoyner-tri can you weigh in with your thoughts? Then we can summarize / decide.
If we are moving model directives into multibody::Parser, do we need to maintain the legacy model directives interface at all? Can we deprecate and eventually delete it?
It's not uncommon for code to have a vector<ModelDirective> already in memory (often created programmatically) that needs to be loaded. We'll want to retain the ability to load that one way or another -- either via a Parser function that accepts that data type, or leaving ProcessModelDirectives as-is. Possibly we could offer some sugar for going through AddAllModelsFromString instead (by serializing the directives vector into a string), but I suspect that would be awkward.
Other relevant piece is that some code sadly still relies on the ModelInstanceInfo details that (optionally) come out of directives processing. For now, we need to preserve that information. It would be plausible to have the Parser be able to regurgitate it, though, if we are switching up the API and want to get rid of the free functions.
My thought so far is to migrate the bulk of the model-directives implementation to detail/internal, and wire up the existing public api to that by trivial forwarding. That will solve my physical design problem, and make it easy to abandon the old api later if we decide it's time.
I'd appreciate thoughts on how to update README_model_directives.md, if we are keeping the old api around. Seems like the claims of "we'll ditch this all RSN" are no longer true. \cc @ggould-tri @EricCousineau-TRI
As for arbitrary compositing of models from multiple files, I wonder if the better answer is to invent some new class (possibly friends with parser, or something) that handles multi-file composites. The existing Parser is oriented around single-file ops (possibly multiple models, through), and I think that's a reasonable scope limit.
Re: the README -- The claim of model directives being "temporary" is still true, but the time constant is not explained well. Open Robotics is actively working on reaching feature parity (#15948, #17223) -- it's just not finished yet.
With the addition of SDFormat includes (already implemented and available in Drake), the Parser already has the responsibility of loading multiple files (and models). I think we should keep the Parser API for users the same. If it makes sense under the hood to refactor into better helper classes, that's fine.
I'm not objecting to inclusions as part of Parser -- that ship has already sailed. The vector-of-files-with-external-joint-glue thing seems out of scope to me.
fwiw -- if this were to land this week, then it would clean up the notebooks that will give students their first look at Drake in class next week. (not necessary, just know that there is interest!)
This issue is complete with the merge of #17623. However, the work raised some further questions that we should track:
- #18059 The "singular" methods are deeply awkward to implement for multi-model capable formats, and perhaps have outlived their usefulness with the ability to scope-name models rather than renaming them. Consider deprecation and removal
- #18060 Related to the previous point, we are missing a "plural" method for string sources. It should be trivial to implement, but needs some testing.
- #18061 There is a deprecation discussion around model directives that needs updating. Do we still intend to deprecate something? Is it the whole format? The older free-function API? What do we need to say about any of it?
I will file new issues for the points above.