Skip to content

Web content embeddings [LAB-18] #105

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

Open
wants to merge 51 commits into
base: develop
Choose a base branch
from

Conversation

nikhiltri
Copy link
Member

No description provided.

nikhiltri added 30 commits June 6, 2025 14:36
…ing since the classes already define a connection [LAB-13]
…earch-in-es

Add vector search to general keyword searches [LAB-11]
…te-embeddings-on-import

Regenerate embeddings on artwork import [LAB-13]
Copy link
Member

@zachgarwood zachgarwood left a comment

Choose a reason for hiding this comment

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

There are a couple bits that are a little confusing.

$this->info("Saved text embeddings", OutputInterface::VERBOSITY_VERBOSE);
} catch (\Exception $e) {
throw new Exception("Failed to save text embeddings: " . $e->getMessage());
}
Copy link
Member

Choose a reason for hiding this comment

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

This try-catch inside a try-catch seems redundant.

}
});

usleep($this->sleepFor * 10000);
Copy link
Member

Choose a reason for hiding this comment

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

In the ImportsData trait, it says that $sleepFor is "Seconds to sleep between requests" but here we're only sleeping for 10 milliseconds (10,000 microseconds = 10 milliseconds). For clarity, maybe we should use the Laravel Sleep helper, for example:

use Illuminate\Support\Sleep;

protected $sleepFor = 1 / 100;  // 0.01 seconds or 10 milliseconds

Sleep::for($sleepFor)->seconds();

Copy link
Member

Choose a reason for hiding this comment

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

@nikhiltri nikhiltri force-pushed the feature/web-content-embeddings branch from ebc0fc1 to ae06694 Compare July 7, 2025 16:16
Copy link

sonarqubecloud bot commented Jul 7, 2025

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