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

Update pybio (bioimageio) submodules #164

Merged
merged 10 commits into from
Jun 10, 2021
Merged

Update pybio (bioimageio) submodules #164

merged 10 commits into from
Jun 10, 2021

Conversation

FynnBe
Copy link
Member

@FynnBe FynnBe commented May 6, 2021

.. and remove unused cache_path from eval_model_zip

FynnBe added 3 commits May 6, 2021 17:00
treating an absolute path on win leads to interpreting the drive letter
as uri scheme...
@FynnBe FynnBe requested a review from k-dominik May 6, 2021 15:07
@FynnBe FynnBe changed the title Update pybio submodules Update pybio (bioimageio) submodules May 7, 2021
@FynnBe
Copy link
Member Author

FynnBe commented May 7, 2021

.. and refactor package name pybio -> bioimageio

problem: torch script weight formats seems to become invalid due to this refactor

Are any bioimageio models currently on the website already use torchscript or can we just go ahead and recreate these test weights and ignore this problem otherwise? @constantinpape , I think you created the torch script weights, what's your take?

@k-dominik
Copy link
Collaborator

problem: torch script weight formats seems to become invalid due to this refactor

out of curiosity - do you know why this happens?

@FynnBe
Copy link
Member Author

FynnBe commented May 18, 2021

problem: torch script weight formats seems to become invalid due to this refactor

out of curiosity - do you know why this happens?

no, but the same happens with pickle. So maybe this is related

@k-dominik
Copy link
Collaborator

ahh @FynnBe, you have good instincts. I have checked the old weights, and those reference the model python file in pybio/torch/models/unet2d.py... So I would assume this is only related to this testdata.

@FynnBe
Copy link
Member Author

FynnBe commented May 18, 2021

So I would assume this is only related to this testdata.

good. The refactor really shouldn't break anything else

@FynnBe
Copy link
Member Author

FynnBe commented May 18, 2021

@k-dominik any idea why the CI doesn't run here?

@FynnBe FynnBe force-pushed the update_pybio branch 2 times, most recently from b52e09b to 61a2285 Compare May 18, 2021 11:56
Copy link
Collaborator

@k-dominik k-dominik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm in general. Test failure seems to be related to torchscript archive. Not sure how this was generated, but given that the size is almost twice as before, there seems to be something odd.

@k-dominik
Copy link
Collaborator

to me it looks like the tests/data/unet2d_torchscript/weights.pt file is corrupt. I tried unzipping it and got the following error:

caution:  zipfile comment truncated
error [.../tiktorch/tests/data/unet2d_torchscript/weights.pt]:  missing 9131070 bytes in zipfile
  (attempting to process anyway)
error [.../tiktorch/tests/data/unet2d_torchscript/weights.pt]:  attempt to seek before beginning of zipfile

@constantinpape , @FynnBe how was this weight file generated? Would it be possible to regenerate it? (By the way, I found it interesting that this information could not be inferred from the model.yaml... - how are users supposed to know how it was trained?)

@constantinpape
Copy link
Member

to me it looks like the tests/data/unet2d_torchscript/weights.pt file is corrupt.

Yes indeed, looks like something with the torchscript weights is wrong.

@constantinpape , @FynnBe how was this weight file generated? Would it be possible to regenerate it?

The whole model was copied from here https://github.com/bioimage-io/pytorch-bioimage-io/blob/master/specs/models/unet2d_nuclei_broad/UNet2DNucleiBroad.model.yaml#L63-L76 and the torchscript weigths are just generated from the normal pytorch model.

(By the way, I found it interesting that this information could not be inferred from the model.yaml... - how are users supposed to know how it was trained?)

Yes, that's indeed a bit unfortunate. The reason is that we had a formalized description of the training procedure in the first iteration of the bioimage format. However, it turned out that this was to complex, so we know rely on self-documentation of the training procedure and providing the training script or linking to the relevant notebook. Unfortunately for the model at hand this got lost over time because it was copied around so much ...
I can re-add this part, but for this it would be good if we use a single version of this model (preferably the one at https://github.com/bioimage-io/pytorch-bioimage-io/blob/master/specs/models/unet2d_nuclei_broad/UNet2DNucleiBroad.model.yaml) and don't keep separate versions that need to be kept in sync.

@k-dominik
Copy link
Collaborator

thank you for the update @constantinpape !

I can re-add this part, but for this it would be good if we use a single version of this model (preferably the one at https://github.com/bioimage-io/pytorch-bioimage-io/blob/master/specs/models/unet2d_nuclei_broad/UNet2DNucleiBroad.model.yaml) and don't keep separate versions that need to be kept in sync.

let's re-add the weights here and tackle the single model change in a separate PR. I've opened an issue (#165) about using a single model for the test case.

@constantinpape
Copy link
Member

constantinpape commented Jun 9, 2021

I think the torchscript weights were generated with this script:
https://github.com/bioimage-io/pytorch-bioimage-io/blob/master/pybio/torch/converters/torchscript.py

Copy link
Collaborator

@k-dominik k-dominik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm,

the only thing I really do not like is that this PR adds even more pip dependencies.

Ideally, we would build the whole tiktorch-server package without pip. I have started the process on making some of the packages available via conda-forge (conda-forge/staged-recipes#15250)

@FynnBe
Copy link
Member Author

FynnBe commented Jun 10, 2021

@k-dominik should we continue this PR a bit more do switch to conda-packages and further update the bioimageio dependencies?

@k-dominik
Copy link
Collaborator

oh I would prefer to merge this now. With conda packages you never know :)

@FynnBe FynnBe merged commit 1181ced into master Jun 10, 2021
@FynnBe FynnBe deleted the update_pybio branch June 10, 2021 09:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants