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

Custom changeset management #50

Merged
merged 4 commits into from
Sep 24, 2024
Merged

Custom changeset management #50

merged 4 commits into from
Sep 24, 2024

Conversation

Chris7
Copy link
Collaborator

@Chris7 Chris7 commented Sep 20, 2024

This is an extension to the changeset management to identify dependencies of a changeset and write those out. On imports, we ensure that the ids of dependent entities are created or retrieved from the current checked out database.

The workflow this handles is suppose we have branch A with operations 1->2->3, and branch B with operations 1->4->5. If we apply operation 3, things required from operation 2 may not be present. This ensures they will be. For the reversion, we create a new commit with the updates from this application (similar to a new commit id resulting from a cherry-pick).

@@ -17,3 +17,6 @@ noodles = { version = "0.78.0", features = ["core", "vcf", "fasta", "async"] }
petgraph = "0.6.5"
chrono = "0.4.38"
tempdir = "0.3.7"
fallible-streaming-iterator = "0.1.9"
serde = { version = "1.0.210", features = ["derive"] }
serde_json = "1.0.128"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

we can pick a more efficient format once we settle on this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

@@ -49,7 +49,7 @@ pub fn parse_genotype(gt: &str) -> Vec<Option<Genotype>> {
true => Phasing::Unphased,
false => Phasing::Phased,
};
for entry in gt.split_inclusive(|c| c == '|' || c == '/') {
for entry in gt.split_inclusive(['|', '/']) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

clippy made me do it

Copy link
Collaborator

Choose a reason for hiding this comment

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

Time to revolt

#[derive(Clone, Debug, Eq, Hash, PartialEq)]
use crate::models::{edge::Edge, path_edge::PathEdge, sequence::Sequence, strand::Strand, Sample};

#[derive(Clone, Debug, Eq, Hash, PartialEq, Deserialize, Serialize)]
pub struct Path {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

reorganized so it matches the order in up.sql

@@ -76,7 +81,33 @@ impl Path {
})
})
.unwrap();
let path = rows.next().unwrap().unwrap();
let path = match rows.next().unwrap() {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

made creation get-or-create

Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

@Chris7 Chris7 requested a review from dkhofer September 23, 2024 15:56
@@ -105,17 +106,29 @@ impl BlockGroup {
Err(rusqlite::Error::SqliteFailure(err, details)) => {
if err.code == rusqlite::ErrorCode::ConstraintViolation {
println!("{err:?} {details:?}");
BlockGroup {
id: conn
let bg_id = match(sample_name) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this was bugged behavior if the insert was conflicting it would always return the null sample

Base automatically changed from cross-branch-operations to main September 23, 2024 17:52
@@ -113,6 +114,7 @@ mod tests {
let mut fasta_path = PathBuf::from(env!("CARGO_MANIFEST_DIR"));
fasta_path.push("fixtures/simple.fa");
let conn = get_connection(None);
let db_uuid = metadata::get_db_uuid(&conn);
Copy link
Collaborator

Choose a reason for hiding this comment

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

There's a duplicate of this line a couple lines down, I think I added that to fix the test

@@ -32,13 +34,28 @@ pub struct EdgeData {
pub phased: i32,
}

impl From<&Edge> for EdgeData {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice, thanks for this

@@ -528,6 +545,79 @@ mod tests {
assert_eq!(edge_result3.target_coordinate, -1);
}

#[test]
fn test_bulk_create_returns_edges_in_order() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, was bulk create not returning them in order? Oops, thanks!

@@ -65,8 +68,10 @@ pub struct NewBlock {

impl Path {
pub fn create(conn: &Connection, name: &str, block_group_id: i32, edge_ids: &[i32]) -> Path {
// TODO: Should we do something if edge_ids don't match here?
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't understand what is meant by "if edge_ids don't match", can you add a bit more? I have a list of a few validations I'd like to add, I'll bring them up on slack

@@ -30,6 +31,17 @@ pub struct NewSequence<'a> {
shallow: bool,
}

impl<'a> From<&'a Sequence> for NewSequence<'a> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for this too

.unwrap()
.to_string();
created_edges.insert(edge_pk);
if !created_sequences.contains(&source_hash) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

How are we sure that this sequence doesn't show up later as a created sequence?

Copy link
Collaborator

@dkhofer dkhofer left a comment

Choose a reason for hiding this comment

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

This is awesome. LGTM, no blocking comments

@Chris7 Chris7 merged commit 88020cd into main Sep 24, 2024
1 check passed
@Chris7 Chris7 deleted the changeset-v2 branch September 24, 2024 14:37
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.

2 participants