From 340968956685a51135ef480ef2522dbdecb86ab0 Mon Sep 17 00:00:00 2001 From: Brandon Thomas Date: Tue, 4 Apr 2023 13:59:21 -0700 Subject: [PATCH] fix path handling, introduce another (smaller scope) filesystem crate --- Cargo.lock | 11 +++ Cargo.toml | 1 + crates/lib/errors/Cargo.toml | 1 + crates/lib/files/src/lib.rs | 2 + crates/lib/filesys/Cargo.toml | 24 ++++++ crates/lib/filesys/src/filename_concat.rs | 78 +++++++++++++++++++ crates/lib/filesys/src/lib.rs | 22 ++++++ crates/lib/filesys/src/path_to_string.rs | 52 +++++++++++++ crates/service/job/download_job/Cargo.toml | 2 + .../job_types/tts/vits/process_vits_model.rs | 3 +- crates/service/job/inference_job/Cargo.toml | 2 + .../tacotron2_inference_command.rs | 25 +++--- 12 files changed, 208 insertions(+), 15 deletions(-) create mode 100644 crates/lib/filesys/Cargo.toml create mode 100644 crates/lib/filesys/src/filename_concat.rs create mode 100644 crates/lib/filesys/src/lib.rs create mode 100644 crates/lib/filesys/src/path_to_string.rs diff --git a/Cargo.lock b/Cargo.lock index 2b76a5141f..5ed1e58bde 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2314,6 +2314,8 @@ dependencies = [ "enums", "envvar", "errors", + "files", + "filesys", "google_drive_common", "hashing", "hostname", @@ -2682,6 +2684,13 @@ dependencies = [ "ring 0.17.0-alpha.11", ] +[[package]] +name = "filesys" +version = "0.0.1" +dependencies = [ + "errors", +] + [[package]] name = "firestorm" version = "0.4.6" @@ -3599,6 +3608,8 @@ dependencies = [ "enums", "envvar", "errors", + "files", + "filesys", "hashing", "hostname", "hyper 0.14.24", diff --git a/Cargo.toml b/Cargo.toml index 2b919adbf2..f9f834f51f 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -30,6 +30,7 @@ members = [ "crates/lib/envvar", "crates/lib/errors", "crates/lib/files", + "crates/lib/filesys", "crates/lib/hashing", "crates/lib/idempotency", "crates/lib/jobs_common", diff --git a/crates/lib/errors/Cargo.toml b/crates/lib/errors/Cargo.toml index c90b83bb80..2e77bf7e75 100644 --- a/crates/lib/errors/Cargo.toml +++ b/crates/lib/errors/Cargo.toml @@ -16,6 +16,7 @@ path = "src/lib.rs" [dependencies] # NB: This is one of the most central crates in the monorepo and everything depends on it. # Consider the dependencies here wisely. +# Ideally we have almost nothing as a dependency of this crate. anyhow = "1.0.40" [dev-dependencies] diff --git a/crates/lib/files/src/lib.rs b/crates/lib/files/src/lib.rs index 92f88ed486..71b19699d0 100644 --- a/crates/lib/files/src/lib.rs +++ b/crates/lib/files/src/lib.rs @@ -1,6 +1,8 @@ //! files //! //! The purpose of this library is to group common file (or in-memory file) operations. +//! This crate is slightly higher level than the "filesys" crate and will contain more +//! dependencies and file-specific features. //! // Never allow these diff --git a/crates/lib/filesys/Cargo.toml b/crates/lib/filesys/Cargo.toml new file mode 100644 index 0000000000..f1ef0dbfb3 --- /dev/null +++ b/crates/lib/filesys/Cargo.toml @@ -0,0 +1,24 @@ +[package] +name = "filesys" +edition = "2018" +version = "0.0.1" +authors = [ + "Brandon Thomas ", + "Brandon Thomas ", +] +publish = false + +[lib] +# NB: Public "filesys" crate does not exist, so it's safe to use for now. +name = "filesys" +path = "src/lib.rs" + +[dependencies] +# Internal +errors = { path = "../../lib/errors" } + +# External +# KEEP THIS TO A MINIMUM OR NIL. + +[dev-dependencies] +# None yet diff --git a/crates/lib/filesys/src/filename_concat.rs b/crates/lib/filesys/src/filename_concat.rs new file mode 100644 index 0000000000..5e01e2f103 --- /dev/null +++ b/crates/lib/filesys/src/filename_concat.rs @@ -0,0 +1,78 @@ +use std::path::Path; +use crate::path_to_string::path_to_string; + +/// Concatenate filenames while preserving path information. Do not join paths as directories. +/// This contains no file normalizing logic, it's purely a concatenation. +/// +/// eg. "foo" + "bar" results in "foobar" instead of the "foo/bar" that Path::join() returns. +/// eg. "/usr/bin" + "baz" results in "/usr/binbaz" instead of "/usr/bin/baz" +/// +/// This is not efficient. +pub fn filename_concat, Q: AsRef>(part_1: P, part_2: Q) -> String { + let mut base = path_to_string(part_1); + let suffix= path_to_string(part_2); + base.push_str(&suffix); + base +} + +#[cfg(test)] +mod tests { + use std::path::{PathBuf}; + use crate::filename_concat::filename_concat; + + // Layout: Base, Suffix, Expected Result + const TEST_CASES : [(&'static str, &'static str, &'static str); 11] = [ + // Absolute + ("/", "foo", "/foo"), + ("/usr/bin", "baz", "/usr/binbaz"), + ("/usr/local", "/bin", "/usr/local/bin"), + // Relative + ("picture", ".jpg", "picture.jpg"), + ("/", "..", "/.."), + ("../", "..", "../.."), + ("./", "bin", "./bin"), + (".", "bar", ".bar"), + // Misc + ("", "", ""), + ("/", "", "/"), + ("", "/", "/"), + ]; + + #[test] + fn pathbuf() { + for (part_1, part_2, expected) in TEST_CASES.iter() { + let test_part_1 = PathBuf::from(part_1); + let test_part_2 = PathBuf::from(part_2); + assert_eq!(filename_concat(test_part_1, test_part_2), expected.to_string()); + } + } + + #[test] + fn path() { + for (part_1, part_2, expected) in TEST_CASES.iter() { + let test_part_1 = PathBuf::from(part_1); + let test_part_2 = PathBuf::from(part_2); + + assert_eq!(filename_concat(test_part_1.as_path(), test_part_2.as_path()), + expected.to_string()); + } + } + + #[test] + fn str() { + for (part_1, part_2, expected) in TEST_CASES.iter() { + let test_part_1 = *part_1; + let test_part_2 = *part_2; + assert_eq!(filename_concat(test_part_1, test_part_2), expected.to_string()); + } + } + + #[test] + fn mixed_type() { + for (part_1, part_2, expected) in TEST_CASES.iter() { + let test_part_1 = PathBuf::from(part_1); + let test_part_2 = *part_2; + assert_eq!(filename_concat(test_part_1, test_part_2), expected.to_string()); + } + } +} diff --git a/crates/lib/filesys/src/lib.rs b/crates/lib/filesys/src/lib.rs new file mode 100644 index 0000000000..65f59f4c1f --- /dev/null +++ b/crates/lib/filesys/src/lib.rs @@ -0,0 +1,22 @@ +//! filesys +//! +//! The purpose of this library is to make basic filesystem operations easier. +//! We won't be including content type or magic type features that incur a higher cost. +//! + +// Never allow these +#![forbid(private_in_public)] +#![forbid(unused_must_use)] // NB: It's unsafe to not close/check some things + +// Okay to toggle +#![forbid(unreachable_patterns)] +#![forbid(unused_imports)] +#![forbid(unused_mut)] +#![forbid(unused_variables)] + +// Always allow +#![allow(dead_code)] +#![allow(non_snake_case)] + +pub mod filename_concat; +pub mod path_to_string; diff --git a/crates/lib/filesys/src/path_to_string.rs b/crates/lib/filesys/src/path_to_string.rs new file mode 100644 index 0000000000..e68bc07385 --- /dev/null +++ b/crates/lib/filesys/src/path_to_string.rs @@ -0,0 +1,52 @@ +use std::path::Path; + +/// Convert a path to a string without adding quotes or garbage. +pub fn path_to_string>(path: P) -> String { + path.as_ref().display().to_string() +} + +#[cfg(test)] +mod tests { + use std::path::{PathBuf}; + use crate::path_to_string::path_to_string; + + const TEST_CASES : [&'static str; 9] = [ + // Absolute + "/", + "/usr/bin", + // Relative + "picture.jpg", + "foo/bar/baz", + "../", + "./", + ".", + // Misc + "", + ".././../foo/.././bar", + ]; + + #[test] + fn pathbuf() { + for example in TEST_CASES.iter() { + let test = PathBuf::from(example); + assert_eq!(path_to_string(test), example.to_string()); + } + } + + #[test] + fn path() { + for example in TEST_CASES.iter() { + let path = PathBuf::from(example); + let test = path.as_path(); + assert_eq!(path_to_string(test), example.to_string()); + } + } + + #[test] + fn str() { + for example in TEST_CASES.iter() { + let test = *example; + assert_eq!(path_to_string(test), example.to_string()); + } + } +} diff --git a/crates/service/job/download_job/Cargo.toml b/crates/service/job/download_job/Cargo.toml index 0162d81c17..ab2466eca0 100644 --- a/crates/service/job/download_job/Cargo.toml +++ b/crates/service/job/download_job/Cargo.toml @@ -21,6 +21,8 @@ container_common = { path = "../../../lib/deprecated/container_common" } enums = { path = "../../../_types/enums" } envvar = { path = "../../../lib/envvar" } errors = { path = "../../../lib/errors" } +files = { path = "../../../lib/files" } +filesys = { path = "../../../lib/filesys" } google_drive_common = { path = "../../../lib/api_clients/google_drive_common" } hashing = { path = "../../../lib/hashing" } jobs_common = { path = "../../../lib/jobs_common" } diff --git a/crates/service/job/download_job/src/job_types/tts/vits/process_vits_model.rs b/crates/service/job/download_job/src/job_types/tts/vits/process_vits_model.rs index 43ed30f64d..8b3eaec2a1 100644 --- a/crates/service/job/download_job/src/job_types/tts/vits/process_vits_model.rs +++ b/crates/service/job/download_job/src/job_types/tts/vits/process_vits_model.rs @@ -18,6 +18,7 @@ use std::fs::File; use std::io::Read; use std::path::{Path, PathBuf}; use tempdir::TempDir; +use filesys::filename_concat::filename_concat; /// Returns the token of the entity. pub async fn process_vits_model<'a, 'b>( @@ -37,7 +38,7 @@ pub async fn process_vits_model<'a, 'b>( let original_model_file_path = PathBuf::from(download_filename.clone()); // NB: We'll be creating the traced model in the "check" step and uploading it to GDrive along with the original. - let traced_model_file_path = PathBuf::from(format!("{:?}_traced", original_model_file_path.as_os_str())); + let traced_model_file_path = PathBuf::from(filename_concat(&original_model_file_path, "_traced")); let config_path = PathBuf::from("configs/ljs_li44_tmbert_nmp_s1_arpa.json"); // TODO: This could be variable. diff --git a/crates/service/job/inference_job/Cargo.toml b/crates/service/job/inference_job/Cargo.toml index 269a4c14a7..ad2451cf1a 100644 --- a/crates/service/job/inference_job/Cargo.toml +++ b/crates/service/job/inference_job/Cargo.toml @@ -21,6 +21,8 @@ container_common = { path = "../../../lib/deprecated/container_common" } enums = { path = "../../../_types/enums" } envvar = { path = "../../../lib/envvar" } errors = { path = "../../../lib/errors" } +files = { path = "../../../lib/files" } +filesys = { path = "../../../lib/filesys" } hashing = { path = "../../../lib/hashing" } jobs_common = { path = "../../../lib/jobs_common" } memory_caching = { path = "../../../lib/caching/memory_caching" } diff --git a/crates/service/job/inference_job/src/job/job_types/tts/tacotron2_v2_early_fakeyou/tacotron2_inference_command.rs b/crates/service/job/inference_job/src/job/job_types/tts/tacotron2_v2_early_fakeyou/tacotron2_inference_command.rs index f509feb4ab..4b4c0a46d6 100644 --- a/crates/service/job/inference_job/src/job/job_types/tts/tacotron2_v2_early_fakeyou/tacotron2_inference_command.rs +++ b/crates/service/job/inference_job/src/job/job_types/tts/tacotron2_v2_early_fakeyou/tacotron2_inference_command.rs @@ -1,4 +1,5 @@ use crate::AnyhowResult; +use filesys::path_to_string::path_to_string; use log::info; use std::path::{Path, PathBuf}; use subprocess::{Popen, PopenConfig}; @@ -108,7 +109,7 @@ impl Tacotron2InferenceCommand { ) -> AnyhowResult<()> { let mut command = String::new(); - command.push_str(&format!("cd {}", path_to_str(&self.tacotron_code_root_directory))); + command.push_str(&format!("cd {}", path_to_string(&self.tacotron_code_root_directory))); if let Some(venv_command) = self.maybe_virtual_env_activation_command.as_deref() { command.push_str(" && "); @@ -123,12 +124,12 @@ impl Tacotron2InferenceCommand { command.push_str(" && "); command.push_str(python_binary); command.push_str(" "); - command.push_str(&path_to_str(&self.inference_script_name)); + command.push_str(&path_to_string(&self.inference_script_name)); // ===== Begin Python Inference Args ===== command.push_str(" --synthesizer_checkpoint_path "); - command.push_str(&path_to_str(args.synthesizer_checkpoint_path)); + command.push_str(&path_to_string(args.synthesizer_checkpoint_path)); command.push_str(" --text_pipeline_type "); command.push_str(args.text_pipeline_type); @@ -139,7 +140,7 @@ impl Tacotron2InferenceCommand { command.push_str("waveglow"); command.push_str(" --waveglow_vocoder_checkpoint_path "); - command.push_str(&path_to_str(waveglow_vocoder_checkpoint_path)); + command.push_str(&path_to_string(waveglow_vocoder_checkpoint_path)); } VocoderForInferenceOption::HifiganSuperres { hifigan_vocoder_checkpoint_path, @@ -149,10 +150,10 @@ impl Tacotron2InferenceCommand { command.push_str("hifigan-superres"); command.push_str(" --hifigan_vocoder_checkpoint_path "); - command.push_str(&path_to_str(hifigan_vocoder_checkpoint_path)); + command.push_str(&path_to_string(hifigan_vocoder_checkpoint_path)); command.push_str(" --hifigan_superres_vocoder_checkpoint_path "); - command.push_str(&path_to_str(hifigan_superres_vocoder_checkpoint_path)); + command.push_str(&path_to_string(hifigan_superres_vocoder_checkpoint_path)); } } @@ -169,16 +170,16 @@ impl Tacotron2InferenceCommand { } command.push_str(" --input_text_filename "); - command.push_str(&path_to_str(args.input_text_filename)); + command.push_str(&path_to_string(args.input_text_filename)); command.push_str(" --output_audio_filename "); - command.push_str(&path_to_str(args.output_audio_filename)); + command.push_str(&path_to_string(args.output_audio_filename)); command.push_str(" --output_spectrogram_filename "); - command.push_str(&path_to_str(args.output_spectrogram_filename)); + command.push_str(&path_to_string(args.output_spectrogram_filename)); command.push_str(" --output_metadata_filename "); - command.push_str(&path_to_str(args.output_metadata_filename)); + command.push_str(&path_to_string(args.output_metadata_filename)); // ===== End Python Inference Args ===== @@ -207,7 +208,3 @@ impl Tacotron2InferenceCommand { Ok(()) } } - -fn path_to_str>(path: P) -> String { - path.as_ref().display().to_string() -}