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 StarDist Plant Nuclei 3D ResNet #649

Merged
merged 2 commits into from
Oct 11, 2023

Conversation

bioimageiobot
Copy link
Collaborator

This is an automatic PR created by the @bioimageiobot regarding changes to the resource item 10.5281/zenodo.8421755.
The following version(s) will be added:

Please review the changes and make sure the new item or version(s) pass the following check list:

  • Passed the bioimage.io CI tests: static (and dynamic) validations
  • The meta information for the RDF item is complete
    • The tags are complete and describe the model
    • Naming is intuitive and descriptive, example: Multi-Organ Nucleus Segmentation (StarDist 2D)
    • Authors are provided
    • Documentation is complete
      • For models, include an overview, describe how the model is trained, what is the training data, how to use the model, how to validate the results and list the references. TODO: Model documentation template.
  • Approved by at least one of the bioimage.io admin team member.

Maintainers: @qin-yu

Note: If you updated or re-uploaded another version for the current item on Zenodo, this PR won't be changed automatically. To proceed, you can do the following:

  1. Block this version, but keep looking for future versions: Edit the current resource.yaml and keep the top-level status field as accepted, but change the status under the current version to blocked.
  2. Accept this version and keep looking for future versions: Merge this PR for now.
  3. Keep proposed version(s) (and this resource in general if it is new) as pending: Close this PR without merging.

Then wait for the CI on the main branch to complete. It should detect the new version(s) and create another PR for the new version(s).

Previous PRs of this resource: #646, #648

@FynnBe
Copy link
Member

FynnBe commented Oct 11, 2023

unfortunately it still fails to install the env

'error': 'Failed to install conda environment:\n'
            'channels: [defaults, conda-forge]\n'
            'dependencies:\n'
            '- python>=3.7,<3.8\n'
            '- bioimageio.core>=0.5.0; extra == "bioimageio"\n'
            '- pip\n'
            '- pip: [stardist>=0.8.3, tensorflow>=1.[15](https://github.com/bioimage-io/collection-bioimage-io/actions/runs/6479536633/job/17593437873?pr=649#step:9:16).3]\n'
            "name: '8429[22](https://github.com/bioimage-io/collection-bioimage-io/actions/runs/6479536633/job/17593437873?pr=649#step:9:23)3'\n",
   'id': 'validation_summary_tensorflow_saved_model_bundle',
   'name': 'install test environment',
   'status': 'failed'

@bioimageiobot
Copy link
Collaborator Author

preview-collection-json

@esgomezm
Copy link
Contributor

Hi @FynnBe
I'm not understanding well the issue here. Is it that it fails installing stardist?? why should we install it?

@FynnBe
Copy link
Member

FynnBe commented Oct 11, 2023

Hi @FynnBe I'm not understanding well the issue here. Is it that it fails installing stardist?? why should we install it?

because the weights are specified as pytorch state dict and the corresponding source code depends on the stardist library...
yes, the issue is that our CI fails to install the conda env specified here that has specific stardist/tensorflow versions. the issue is somewhat unrelated to our CI and more about getting a valid conda env with suitable stardist/tf versions...
mamba env create --file env.yaml fails for the specified env file

@FynnBe FynnBe merged commit 2608d38 into main Oct 11, 2023
@FynnBe FynnBe deleted the auto-update-10.5281/zenodo.8421755 branch October 11, 2023 12:10
@esgomezm
Copy link
Contributor

Ok! I reviewed the model. This model has nothing to do with pytorch at all and you don't need any stardist dependency to run it (unless you want to use it in stardist, i.e. no dependencies except tensorflow needed). The tensorflow version is already specified in the weights. @FynnBe could you please try running the CI without the dependencies or @qin-yu remove stardist and all sorts of unneeded dependencies from the environment?

@FynnBe
Copy link
Member

FynnBe commented Oct 11, 2023

thanks @esgomezm for taking a closer look!
great if only TF is required! @qin-yu please remove the dependencies if they are not actually required for inference

@qin-yu
Copy link

qin-yu commented Oct 11, 2023

Thanks @esgomezm and @FynnBe

bioimageio test-model passes without a dependency file. I thought I could just edit the Zenodo record (version 3) to remove a line and delete the env config, but it seems that this is not allowed. Since I can't avoid publishing yet another minor fix, I'll do another upload to update the model to version 4?

Note: File addition, removal or modification are not allowed after an upload has been published. This is because a Digital Object Identifier (DOI) is registered with DataCite for each upload. If you've made a mistake please contact us.

@esgomezm
Copy link
Contributor

yes, please upload the new version and click on "update the model" when it suggests that. The system will automatically create a new version. Also, you asked about new DOIs before: yes, a new doi is created because DOIs are unique identifiers of a particular contribution (papers, models or anything with a DOI). If you change the model, even for updating it, a new DOI is assigned because the model is somehow different.

@qin-yu
Copy link

qin-yu commented Oct 11, 2023

Thanks @esgomezm

Zenodo keeps environment.yaml for me after uploading files without this one. Is it expected? I manually clicked the bin icon next to it in Zenodo interface and clicked "save" then go back to bioimage.io and publish. I hope this time it passes the CI.

@uschmidt83
Copy link

It would be nice if one of you could comment on stardist/stardist#254. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants