Skip to content
This repository has been archived by the owner on Oct 9, 2023. It is now read-only.

Fiftyone Image embedder example broken #1031

Closed
ethanwharris opened this issue Dec 6, 2021 · 6 comments
Closed

Fiftyone Image embedder example broken #1031

ethanwharris opened this issue Dec 6, 2021 · 6 comments
Assignees
Labels
bug / fix Something isn't working documentation Improvements or additions to documentation help wanted Extra attention is needed
Milestone

Comments

@ethanwharris
Copy link
Collaborator

🐛 Bug

Following the addition of the VISSL training strategies to the image embedder, the FiftyOne image embedding example is no longer correct.

@ethanwharris ethanwharris added bug / fix Something isn't working documentation Improvements or additions to documentation help wanted Extra attention is needed labels Dec 6, 2021
@ethanwharris ethanwharris added this to the v0.7 milestone Jan 31, 2022
@ethanwharris ethanwharris modified the milestones: v0.7, v0.8 Feb 14, 2022
@ar90n
Copy link
Contributor

ar90n commented Mar 25, 2022

Hi there.
I'm so interested in this issue. If anyone is not assigned this issue yet, please assign me.
For now, I have regenerated this issue in my local environment and fixed it with ugly codes.

In my environment, the vanilla image embedding example whose path is flash_example/image_embedder.py doesn’t work too. I think this is caused by the same reason as the fiftyone integration example.

@ethanwharris
Copy link
Collaborator Author

@ar90n Awesome! We have some fixes for the image embedder that are about to go out, but ideally we would remove the current dependency on vissl altogether (this should only be required if you want to use the SSL training support). I think the two changes we need to make are:

  1. add support to the vissl integration to use any backbone from the ImageClassifier backbones (rather than just the vissl backbones it supports at the moment).
  2. make the training_strategy argument and dependency on vissl optional and only required if you want to train / val / test with the ImageEmbedder

I have a PoC for the first one, so I can finish that up then you could take the second? Let me know if that sounds good to you, I'll assign us both to the issue 😃

@ar90n
Copy link
Contributor

ar90n commented Mar 31, 2022

@ethanwharris
Thanks for your reply and for assigning this issue to me. I agree with your suggestions completely. I'll try to make a PR about the second change you said above. In fact, I also felt that ImageEmbedder is too coupled with vissl.

@ar90n
Copy link
Contributor

ar90n commented Apr 4, 2022

@ethanwharris
I created a new PR about the second change you said above based on #1264. I will change its status to Ready for review after #1264 is merged. If I misunderstand what shoud I do, please comment. I'll fix it.

@ethanwharris
Copy link
Collaborator Author

@ar90n Awesome!!! Will take a look and get back to you 😃

@ethanwharris
Copy link
Collaborator Author

Closing this as it was fixed by #1276

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug / fix Something isn't working documentation Improvements or additions to documentation help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

2 participants