Ricard and Dom's work on low system memory in rusty-plants #46
Conversation
There was a problem hiding this comment.
I don't mean to tare this to shreds. Maybe there was a communication error on my part but this isn't really what I was expecting to see. But I also don't totally know what the original bounty said. So maybe this is correct. But its a significant diverge from our current plans. There are some things I would fix. mostly the reoccurring querying of unnecessary information. But even if that is changed I don't think this is a good thing to merge as its function is still based on a fundamental design change that will clash from our other work.
I think some analysis is needed before further action.
There was a problem hiding this comment.
This is a mistake that should be corrected.
rusty-plants/src/main.rs
Outdated
| let num_chunks = (total_bases as f64 / max_chars as f64).ceil() as usize; | ||
|
|
||
| for chunk in 0..num_chunks { | ||
| let mut st = SuffixTree::new(); |
There was a problem hiding this comment.
This is a massive change. This now makes a bunch of small suffix trees. A huge pivot from the current design and is not a desired outcome.
Now. If we weren't trying to get info into a database and wanted one suffix tree I would recommend this but as it stands this isn't the direction the project is heading in. I believe we talked about this.
There was a problem hiding this comment.
Your right Moved the Suffix tree outside, tried to brute force to get different error(or when Rust Kills the process thus relocated here).
rusty-plants/src/utils/mem_usage.rs
Outdated
There was a problem hiding this comment.
While I agree that encapsulation is good. does process_chunk pertain to memory usage? No. So it probably doesn't live in this file. its also not a utility. its a massive part of the main loop. I suggest moving this into main and it being a helper function given the current organization of the project.
There was a problem hiding this comment.
moved
Do you think in side the process_chunk could save the chunk in db or at least store it in memory before appending the rest of results?
rusty-plants/src/main.rs
Outdated
| println!("{:?}, read_length: {}", best, &fragments[i].bases().len()); | ||
|
|
||
| drop(st); | ||
| std::thread::sleep(std::time::Duration::from_secs(10)); |
There was a problem hiding this comment.
I cannot think of any reason we would do that much computation and then tell the program to sleep for an entire 10 seconds.
There was a problem hiding this comment.
Added a delay to reduce the load on the system. I assumed it will allow time for other operational resources to stabilize.
rusty-plants/src/main.rs
Outdated
| println!("Processing chunk from {} to {}", start, end); | ||
| process_chunk(&transcriptome, &mut st, start, end); | ||
|
|
||
| let sys = sysinfo::System::new_all(); |
There was a problem hiding this comment.
This redefines sysinfo multiple times. and to my current knowledge requires syscalls which is slow. and grabs WAY more information then necessary. It should not be in this loop.
I feel this should be a large part of what mem_usage.rs should be managing.
(on further reading syscalls is maybe the wrong word. On supported operating systems it looks for ways that info is stored. On linux for example it checks /proc/meminfo if just memory info. so basically, it checks a bunch of files and reads that info in. so its a bunch of file IO that is unnecessary, which is syscalls. but I felt I should elaborate).
There was a problem hiding this comment.
I mentioned singletons at some point. this would be why.
There was a problem hiding this comment.
relocated outside after st = SuffixTree:new()
regarding singletons you mean smth like for global accesibility?
lazy_static! {
static ref SYSINFO: Mutex<System> = Mutex::new(System::new_all());
}
| } | ||
| println!("{:?}, read_length: {}", best, &fragments[i].bases().len()); | ||
|
|
||
| drop(st); |
There was a problem hiding this comment.
If this were instead loading that information do the disk, It would have been what me and Gavin proposed months ago instead of the database. Which is reassuring in a sense. But is also not what we are trying to do. Then threading those chunks. But its a lot of computation...
There was a problem hiding this comment.
So doing stuff in chunks and droping SuffixTree is a no go?
Mhm maybe something to do parallel processing i think rayon can do it. Then again don't want to add complexity.
| println!("Memory usage after processing chunk {}: {} KB", chunk, sys.used_memory()); | ||
|
|
||
| println!("Aligning fragments"); | ||
| for i in 0..fragments.len() { |
There was a problem hiding this comment.
with it now being multiple suffix trees were now checking this against multiple times. A natural consequence.
There was a problem hiding this comment.
Sufix tree and sys moved outside for loop.
|
Does this compile and run the same as before? How would we test the desired behavior? (Program exists cleanly and drops the suffix tree if it reaches the magic number of 80% of system memory). |
|
I think I understand more about this task. The bounty and task was originally to just detect when system memory is low, print out a message, and gracefully exit, dropping the one big suffix tree. I think this code implements the next step, along with low system memory detection, which is processing suffix trees in chunks from the transcriptome / genome, and perhaps sizing the chunk so that we never run out of memory. However, it appears we still drop them all in the end. I'm merging it to the |
learner-long-life
merged commit 27b6351
into
TheEvergreenStateCollege:mozes-sysmem-branch
No description provided.