-
Notifications
You must be signed in to change notification settings - Fork 117
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
[WIP] core: qdrant_migrator #2571
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks great
format!("ds_{}", self.internal_id) | ||
} | ||
|
||
pub async fn setup( | ||
pub async fn update_config( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only thing we ever want to update is qdrant_config
no ? Would it be better to only expose that ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The store function will remain the same so better align this one as it can be reused
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would have kept the sore
fn flexible, but only expose what you can actually change in update_config
as this is the interface for the rest of the app.
It's "clear" that you can do "unsupported" things by calling store
, but to me data_source
functions shouldn't leave things in an inconsistent state
@@ -10,16 +12,37 @@ use serde::{Deserialize, Serialize}; | |||
pub enum QdrantCluster { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I think we can [serde(rename_all = "kebab-case")]
static QDRANT_CLUSTER_VARIANTS: &[QdrantCluster] = | ||
&[QdrantCluster::Main0, QdrantCluster::Dedicated0]; | ||
|
||
impl ToString for QdrantCluster { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
out of scope but I am suprised that we have to implement these ourselves everytime, there must be a serde (or other) thing to do this for us
core/bin/qdrant_migrator.rs
Outdated
}, | ||
} | ||
|
||
/// A fictional versioning CLI |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure I understand what is fictional about it, wdym ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cargo cult from a lib
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lol
let args = Cli::parse(); | ||
|
||
let rt = tokio::runtime::Builder::new_multi_thread() | ||
.worker_threads(32) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not leave the default ? it will pick the # of cores available on the system.
I don't think we gain anything from having 32 since we'll be running this on machines that don't have many cores.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is aligned with dust_api it's not only about the core and we know the default can be a foot gun if some things are blocking in unexpected ways
// the embeedding size (which is what happens here). May need to be revisited in | ||
// future. | ||
let mut credentials = Credentials::new(); | ||
credentials.insert("OPENAI_API_KEY".to_string(), "foo".to_string()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is a little bit of a code smell, but understandable if we don't want to fix
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is but it will error if it becomes irrelevant (eg we use another provider that require a credentials to perform this function)
core/bin/qdrant_migrator.rs
Outdated
project_id, | ||
data_source_id, | ||
} => { | ||
// This is the most dangerous command of all as it is the only one to actually |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this comment is wrong
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's accurate
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isn't the clear
command the most dangerous one ?
|
||
let qdrant_client = qdrant_clients.main_client(&ds.config().qdrant_config); | ||
|
||
// Delete collection on shadow_write_cluster. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
moved 3 lines below
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this function actually deleting ? I thought this one was copying ?
Tested locally (log below). A document was added and another one deleted during the migration.