Remove planning task composer input_instruction

[marip8]

The PlanningTaskComposerProblem class has a member called input_instruction that presumably holds the instruction that the first task is supposed to use as input.

tesseract_planning/tesseract_task_composer/planning/include/tesseract_task_composer/planning/planning_task_composer_problem.h

Line 90 in d2b11de

InstructionPoly input_instruction;

This seems unecessarily redundant because each task, including the first task, can specify an input key in the DataStorage object from which to extract its relevant input. We should consider removing the input_instruction field in favor of using the DataStorage object. I have tested this in the SNP repo here and it works as expected.

A quick code search shows that only the process planning input task and a unit test seem to use this member anyway.

[Levi-Armstrong]

This decision was to have a clear distinction between the input to the planning pipeline and the data generated as part of the pipeline. I want the problem to hold all required data to initiated a pipeline required for logging/serialization. This way you only have to serialize and log the problem to allow recreating failed or undesirable plans.

[Levi-Armstrong]

This also has other nice properties from the Tesseract Studio perspective for creating universal debug and planning tooling.

[marip8]

I want the problem to hold all required data to initiated a pipeline required for logging/serialization

I don't think this is realistically achievable with only the input_instruction field. There could be tasks in any part of the pipeline that want to look up pre-populated data in the DataStorage class. Also the first task may want some type of data that is not an InstructionPoly type (for instance a mesh file, if the pipeline includes tool path planning and motion planning).

[Levi-Armstrong]

It is realistic because I am doing this for a four different application from motion planning, image processing, mesh processing, and toolpath process, etc. You start by create a data structure which adheres to the InstructionPoly interface that holds all relevant input data, which can be anything you want. Then the first step in the pipeline is a task that you create which processes this instruction and populate the data storage how you see fit. This provides a clean interface via ros, ros2, grpc, etc where it does not need to be task/application aware. The goal is to prevent the need for user to have to create there own task composer server node, where they just launch it with there config yaml file and the message interface is the same regardless of the task at hand.

[marip8]

Makes sense from that perspective. But in this paradigm, it feels unnecessary and redundant to also require the user to provide a data storage object to the executor run method then if all of the input data is captured in the problem object and the tasks interact with the data storage internally.

My main point here is that there are multiple (and possibly conflicting) ways of getting data to tasks, and it is not clear from the code which approach is preferred and which approach takes precedence if someone were stash input data in both places due to a lack of understanding.

If we want the task composer to function per your previous post (which is nominally fine by me), then I think we should do the following to make that paradigm more clear:

• Move input_instruction to the base level TaskComposerProblem such that all types of task composer problems must utilize it for input data
  • Add documentation about what this represents (i.e., structure of all input data that could be populated into a DataStorage
• Add a task that directly stores this input instruction into a named key inside the data storage for straightforward use-cases
• Remove the data storage argument from the task executor run method
• Make the data storage object accessible from TaskContext

[Levi-Armstrong]

I agree, at the time when I was refactoring it I was not sure if there was a use case that it would be need but I have not came across anything.

[marip8]

Okay. I can close this issue and open another with the specific changes. This also feels like something that could be added to the roadmap as a near term minor release, so I can put these tasks on the Github project instead if that makes sense

[Levi-Armstrong]

Sounds, good add it to the github project.

[marip8]

I just added this issue to the roadmap, so it needs to remain open until it's complete. The tasks required to close are in the comments above.

If we're moving input_instruction to the base Problem class, should it still be an Instruction type or should we make it a tesseract::AnyPoly?

[Levi-Armstrong]

If we're moving input_instruction to the base Problem class, should it still be an Instruction type or should we make it a tesseract::AnyPoly?

I think AnyPoly makes sense.

[Levi-Armstrong]

Addressed by #407