Conversation
- It is useful for tensors to contain the information of the spec id, that corresponds to the bioimage tensor spec, to be aware of valid shapes for both inputs and output
thodkatz
force-pushed
the
remove-interface-model-info
branch
from
a242db4 to
fcf3cdb
Compare
thodkatz
force-pushed
the
remove-interface-model-info
branch
from
fcf3cdb to
127d162
Compare
There was a problem hiding this comment.
Great to see ModelInfo gone. I'm sure this will simplify future developments quite a bit. Also, awesome to see some green tests :)
I had a few comments. I guess in summary there is a lot of code that is concerned with validation of the spec. I have not checked the spec library, but is this where the spec is lacking and you implemented a hardened check?
I also appreciate small details, like adding parameter names which makes things so much easier to read.
- Use the class `Sample` to associate xr.DataArray with tensor id coming from the model - Do the checks only from the client side - Add checking about axes validity - Add tests
k-dominik
left a comment
k-dominik
left a comment
There was a problem hiding this comment.
I think this is almost there - my only issue really is holding a reference to prediction pipeline in the model session, which is not really needed for shape validation.
thodkatz
force-pushed
the
remove-interface-model-info
branch
from
4343a07 to
85a4c8c
Compare
k-dominik
left a comment
k-dominik
left a comment
There was a problem hiding this comment.
Okay this looks good. Your changes improved readability for me and it's great that shapes of tensors will now be validated before feeding them through the network :)
tiktorch/converters.py
Outdated
| return Sample({tensor.tensorId: pb_tensor_to_xarray(tensor) for tensor in pb_tensors}) | ||
|
|
||
| @classmethod | ||
| def from_xr_tensors(cls, tensor_ids: List[str], tensors_data: List[xr.DataArray]): |
There was a problem hiding this comment.
| def from_xr_tensors(cls, tensor_ids: List[str], tensors_data: List[xr.DataArray]): | |
| def from_xr_tensors(cls, tensor_ids: List[str], tensors_data: List[xr.DataArray]) -> Sample: |
for completeness
There was a problem hiding this comment.
Yep, I have to add pyright in the pre-commit hooks, so we can catch these as well. Thank you for point it out :)
… bio model client
thodkatz
force-pushed
the
remove-interface-model-info
branch
from
85a4c8c to
da79ae0
Compare
|
Nice! Thank you very much for the review @k-dominik. It was very insightful :) |
2 participants
Following up the discussion #213 (review), we have decided that there is no need to maintain an additional abstract layer in tiktorch (
ModelInfo) as a bioimage io interface, because tiktorch and ilastik are tightly coupled with bioimage spec and core.Thus, in this PR,
ModelInfowas removed. The information provided by theModelInfo, now is represented by the actual bioimage io interface, and more specifically thePredictionPipelinethat contains all the information needed for shapes, axes, names for input and output tensors.The
PredictionPipelineas the new entity, is exposed both from the client and the server, when we initiate a model session process.Althoug the plan is to migrate to spec v5, spec v4 (#212 (comment)) with the concept of parameterized and explicit shape inputs, could be still useful as a baseline. Thus I have added some checks to validate the shapes of the incoming tensors based on the valid shapes provided by the v4 of
PredictionPipeline. These checks will be also useful for the upcoming PR of checking GPU out-of-memory