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

[PR] Fixing reconnection error and adding image list #17

Merged
merged 30 commits into from
Nov 29, 2023
Merged

[PR] Fixing reconnection error and adding image list #17

merged 30 commits into from
Nov 29, 2023

Conversation

ndrean
Copy link
Collaborator

@ndrean ndrean commented Nov 15, 2023

closes #11
closes #7

Copy link

codecov bot commented Nov 15, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (a3c8c3b) 100.00% compared to head (9d38d67) 100.00%.
Report is 7 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff            @@
##              main       #17   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            2         2           
  Lines           32        54   +22     
=========================================
+ Hits            32        54   +22     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ndrean ndrean marked this pull request as draft November 15, 2023 15:01
Copy link
Member

@LuchoTurtle LuchoTurtle left a comment

Choose a reason for hiding this comment

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

It's obviously still a draft PR.

image

But the changes look awesome to me so far! Thanks for also adding a test to keep the coverage to 100%!

If you want me to finish this PR for ya so we can get it merged, just say so :D. Otherwise, we'll wait.

.bumblebee/README.md Outdated Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
@nelsonic nelsonic added the merge-conflicts The branch or pull request has merge conflicts with the target branch (e.g. master) label Nov 20, 2023
@ndrean
Copy link
Collaborator Author

ndrean commented Nov 20, 2023

Sorry for my late response, I am out. Yes of course feel free to modify this if you are happy with this. I wasn't sure it was in line with what you really expected. The HTML is really bad 😥 and the Readme needs to be modified as well. Was just a draft for you to see. I added tests because I wanted to learn this from your code, and the "receive" trick is nice!. I also didn't use the BLIP because the other model is simply faster for dev, so the model handling is not up-to-date, nor deployable with all the modifications you made.

You could also use random images from unsplash and have the joy to discover new images every time as well as the descriptions. This is a real good test and is more fun.

#page_live.ex, lines 5-7
@unsplashes [
    "https://source.unsplash.com/random/300x300",
    "https://source.unsplash.com/random/310x310"
  ]

I also thought that instead of a delayed display of examples, you could add a click button "Show me examples" beneath to show the examples as a component: instead of a handle_info (the response to the Process_send_after), you would use a handle_event. I did not test it but I think that's all it takes for this.

Lastly, I have another branch that can be seen as a "nice" extension I upload to S3, save the response URL in an SQLite db once the caption job is done. The idea is to implement a speech-to-text and a full-text-search (usper easy in SQLite since FS5 is already embedded) to find images from the DB whose caption contains some tags. A simple an input as text of a tag/keyword can run a direct full-text-search in the caption field. You may want to improve this and play with the ML capabilities of Bumblebee: you add a speech-to-text (a Javacsript hook to enable the microphone and save the audio into a blob, and upload to the server (just few lines of JS). Once the server receives the audio binary, a ML model will transcript it into text (Whisper, based on this or here and here). Then another model will perform a keyword extraction / token classification. For example, check/run the exampleBERT 1 on Huggingface ro BERT 2. The last one is implemented in Bumblebee I believe. Once you have an array of keywords, you can use the full-text-search to find matching captions in the db.

@nelsonic
Copy link
Member

@ndrean you have write access:
ndrean-write-access

if you can push your branch directly to the remote: [email protected]:dwyl/image-classifier.git
we can help with tests and docs to get this merged. 👍
(you may need to create a new PR from the branch pushed to this repo ...)

@LuchoTurtle LuchoTurtle self-assigned this Nov 21, 2023
@LuchoTurtle LuchoTurtle added enhancement New feature or enhancement of existing functionality in-progress An issue or pull request that is being worked on by the assigned person labels Nov 21, 2023
# Conflicts:
#	.gitignore
#	lib/app/application.ex
#	lib/app_web/live/page_live.ex
#	mix.lock
@nelsonic
Copy link
Member

@LuchoTurtle thanks for picking this up to enhance and test. 🙏
If the Unsplash (fetching an image) is proving difficult to test,
we can always hard-code the sample images instead of getting them from an API.
(which would actually be preferable for this simple example ...)

Ping me if you need anything from me or when you want a review. 👌

@LuchoTurtle
Copy link
Member

Should be reviewable 👌

@LuchoTurtle LuchoTurtle marked this pull request as ready for review November 28, 2023 13:21
@LuchoTurtle LuchoTurtle changed the title Unsplash & reconnection [PR] Fixing reconnection error and adding image list Nov 28, 2023
@ndrean
Copy link
Collaborator Author

ndrean commented Nov 28, 2023

I hope you don't mind if I open a quick discussion about how to render the example mages. You send base64 strings to populate the src of the tag. This is mandatory when a user uploads an image because the server consumes the browser's upload. But can't we send the URL string instead, only for the examples?
This save CPU since it avoids the conversion to a base64 (which increases the original size in kB) and decreases the socket memory footprint.
I know both the server and browser will download the images as you need to produce a caption, but this frees some CPU and memory.

And sorry for having started something and didn't finish it but I am away since more than a week...

@LuchoTurtle
Copy link
Member

Of course I don't mind! I'm learning the most with your feedback!

You're completely right, I should have reused the the URL instead of base64 encoding the images, it's utterly unnecessary. It went right over my head, thanks you.

And it's okay! Your PR laid the groundwork for the PR, it helped a ton!

@LuchoTurtle LuchoTurtle added in-progress An issue or pull request that is being worked on by the assigned person and removed awaiting-review An issue or pull request that needs to be reviewed labels Nov 28, 2023
@ndrean
Copy link
Collaborator Author

ndrean commented Nov 28, 2023

To get the URL, it is a matter of merging the URL into the (Image-To-Text) task map, after you upload the image with Req.get!. You then have the URL that corresponds to the result when you get it back. Otherwise, it might be complicated to match the URL with the response. I hope I am clear 🤔

@ndrean
Copy link
Collaborator Author

ndrean commented Nov 28, 2023

And I am the one who learns with your tests!

@LuchoTurtle
Copy link
Member

To get the URL, it is a matter of merging the URL into the (Image-To-Text) task map, after you upload the image with Req.get!. You then have the URL that corresponds to the result when you get it back. Otherwise, it might be complicated to match the URL with the response. I hope I am clear 🤔

Yeah, I understand. I remember why I was base64 encoding the images. It's because the https://source.unsplash.com/random/ URL resolves to a different path every time it's called, so the images that were displayed to the user were not the same that was fed into the model. I tried following the redirects but I'm having trouble getting the URL it resolves to.

@ndrean
Copy link
Collaborator Author

ndrean commented Nov 28, 2023

Can't this fix it?

tasks = for _ <- 1..2 do
    {:req, body} = {:req, Req.get!(random_image).body}
    predict_example_image(random_image, body)
                              ^^^
end

[...]
def predict_example_image(rand_url, body) do
                           ^^^
    with {:vix, {:ok, img_thumb}} <-
           {:vix, Vix.Vips.Operation.thumbnail_buffer(body, @image_width)},
         {:pre_process, {:ok, t_img}} <- {:pre_process, pre_process_image(img_thumb)} do

      # Create an async task to classify the image from unsplash
      Task.Supervisor.async(App.TaskSupervisor, fn ->
        Nx.Serving.batched_run(ImageClassifier, t_img)
      end)
      |> Map.merge(%{url: rand_url})
                        ^^^

    else
      {stage, error} -> {stage, error}
    end
  end

@LuchoTurtle
Copy link
Member

It doesn't because the https://source.unsplash.com/random/ link will resolve to a random image in the LiveView and then another random image in the view.

I need to find a way to get the URL after https://source.unsplash.com/random/ resolves to and use that URL instead. If I click on https://source.unsplash.com/random/ on two different tabs, two different images will appear. For example:

https://images.unsplash.com/photo-1699129904144-d295cc270424?crop=entropy&cs=tinysrgb&fit=max&fm=jpg&ixid=MnwxfDB8MXxyYW5kb218MHx8fHx8fHx8MTcwMTIwMTE5OQ&ixlib=rb-4.0.3&q=80&w=1080

https://images.unsplash.com/photo-1698871538204-3e2cf1336d4f?crop=entropy&cs=tinysrgb&fit=max&fm=jpg&ixid=MnwxfDB8MXxyYW5kb218MHx8fHx8fHx8MTcwMTIwMTIwMA&ixlib=rb-4.0.3&q=80&w=1080

^ These are the links I need. I need to follow the redirection, but req doesn't have a way to get the URL https://source.unsplash.com/random/ redirects into.

@ndrean
Copy link
Collaborator Author

ndrean commented Nov 28, 2023

oh I see. Finch does this:

def rand_splash do
%{scheme: scheme, host: host, path: path} = 
    Finch.build(:get, "https://source.unsplash.com/random/")
    |> Finch.request!(MyFinch)
    |> Map.get(:headers)
    |> Enum.filter(fn {a, _b} -> a == "location" end)
    |> List.first()
    |> elem(1)
    |> URI.parse()

   scheme <> "://" <> host <> path
end
App.rand_splash()
# https://images.unsplash.com/photo-1694813646634-9558dc7960e3

App.rand_splash()
# https://plus.unsplash.com/premium_photo-1698846878442-4f0b4166fb85

@LuchoTurtle
Copy link
Member

Thanks for the function. I've added it to the README.
So as to not add a new dependency, I've implemented a different way with the req library that was already installed.

Thanks for the feedback 👌

@LuchoTurtle LuchoTurtle assigned nelsonic and unassigned LuchoTurtle Nov 28, 2023
@LuchoTurtle LuchoTurtle added awaiting-review An issue or pull request that needs to be reviewed and removed in-progress An issue or pull request that is being worked on by the assigned person labels Nov 28, 2023
@ndrean
Copy link
Collaborator Author

ndrean commented Nov 28, 2023

Only use Finch and remove Req, no?

@nelsonic nelsonic added in-review Issue or pull request that is currently being reviewed by the assigned person and removed awaiting-review An issue or pull request that needs to be reviewed labels Nov 29, 2023
let inactivityTimer;
let processHasBeenSent = false;

let ctx = this
Copy link
Member

Choose a reason for hiding this comment

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

Can't say I understand the need to set ctx = this variable ... 🤷‍♂️
Please consider leaving a clarifying comment.

Copy link
Member

Choose a reason for hiding this comment

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

I was having trouble sending the pushEvent otherwise. The this inside the resetInactivityTimer function yielded undefined when I tried pushing the event every time. So I had to get the this from the mounted function instead.

I'll add a clarifying comment in the next PR.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, the context isn't inferred from the parent function, sadly. Comment very much appreciated. Thanks. 😊

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The joy of this in context with JS. Take a look at my draft code for Speech-To-Text "Audio" hook.

Copy link
Member

@nelsonic nelsonic left a comment

Choose a reason for hiding this comment

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

@ndrean & @LuchoTurtle this looks good. 🙌
Happy to merge. :shipit:

@nelsonic nelsonic merged commit 9cb6674 into dwyl:main Nov 29, 2023
3 checks passed
@ndrean
Copy link
Collaborator Author

ndrean commented Nov 29, 2023

Ah, good! You made it with Req, nice! ✌️.

If you really want to showcase Finch, I think you should shorten the snippet to:

def get_redirected_url do
    Finch.build(:get, @url)
    |> Finch.request!(MyFinch)
    |> Map.get(:headers)
    |> Enum.filter(fn {a, _b} -> a == "location" end)
    |> List.first()
    |> elem(1)

The rest of the code was here to extract the path without the query string.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation enhancement New feature or enhancement of existing functionality in-review Issue or pull request that is currently being reviewed by the assigned person
Projects
Status: Done
Status: Done
Development

Successfully merging this pull request may close these issues.

Chore: "We can't find the internet ... Attempting to reconnect" Feat: Examples on the Landing Page!
3 participants