Serde revamp and anyhowization

[calbaker]

No description provided.

[kylecarow]

@calbaker check out the serde api changes ive been playing with! This PR kind of ballooned into more than just anyhowizing things...

Improvements:

• Python-side calls to the to_file and from_file methods now also accept pathlib.Paths, in addition to strings

  • example:

    # %%
    import fastsim as fsim
    from pathlib import Path
    
    # %%
    filepath = "hwfet.csv"
    fsim.fsr.RustCycle.from_file(filepath)
    # returns <fastsimrust.RustCycle at 0x12cf84430>
    
    # %%
    filepath = Path("hwfet.csv")
    fsim.fsr.RustCycle.from_file(filepath)
    # returns <fastsimrust.RustCycle at 0x12cf84530>

  • see how it works here:
    

    fastsim/rust/fastsim-core/fastsim-proc-macros/src/add_pyo3_api/mod.rs

    Lines 108 to 121 in d0a15ae

    	if !is_state_or_history {
    	py_impl_block.extend::<TokenStream2>(quote! {
    	#[pyo3(name = "to_file")]
    	pub fn to_file_py(&self, filepath: &PyAny) -> anyhow::Result<()> {
    	self.to_file(PathBuf::extract(filepath)?)
    	}
    	#[staticmethod]
    	#[pyo3(name = "from_file")]
    	pub fn from_file_py(filepath: &PyAny) -> anyhow::Result<Self> {
    	Self::from_file(PathBuf::extract(filepath)?)
    	}
    	});
    	}

• Rust-side calls to the same methods now accept Rust Path objects as well as strings, because I made the argument generic:
  

  fastsim/rust/fastsim-core/src/traits.rs

  Lines 22 to 23 in d0a15ae

  	fn to_file<P: AsRef<Path>>(&self, filepath: P) -> anyhow::Result<()> {
  	let filepath = filepath.as_ref();

  
  

  fastsim/rust/fastsim-core/src/traits.rs

  Lines 54 to 55 in d0a15ae

  	fn from_file<P: AsRef<Path>>(filepath: P) -> anyhow::Result<Self> {
  	let filepath = filepath.as_ref();

But wait, there's more!

New methods (not yet Python exposed):

• from_reader:
  

  fastsim/rust/fastsim-core/src/traits.rs

  Lines 98 to 104 in d0a15ae

  	fn from_reader<R: std::io::Read>(rdr: R, format: &str) -> anyhow::Result<Self> {
  	Ok(match format {
  	"yaml" => serde_yaml::from_reader(rdr)?,
  	"json" => serde_json::from_reader(rdr)?,
  	_ => anyhow::bail!("Unsupported file format: {format:?}"),
  	})
  	}

  • accepts anything that implements std::io::Read, for example std::fs::File, TCP streams, etc.
  • this method is what from_file now uses and offloads extension matching to (see line 73):
    

    fastsim/rust/fastsim-core/src/traits.rs

    Lines 54 to 76 in d0a15ae

    	fn from_file<P: AsRef<Path>>(filepath: P) -> anyhow::Result<Self> {
    	let filepath = filepath.as_ref();
    	let extension = filepath
    	.extension()
    	.and_then(OsStr::to_str)
    	.with_context(|| {
    	format!(
    	"File extension could not be parsed: \"{}\"",
    	filepath.display()
    	)
    	})?;
    	let file = File::open(filepath).with_context(|| {
    	if !filepath.exists() {
    	format!("File not found: \"{}\"", filepath.display())
    	} else {
    	format!("Could not open file: \"{}\"", filepath.display())
    	}
    	})?;
    	// deserialized file
    	let mut deserialized = Self::from_reader(file, extension)?;
    	deserialized.init()?;
    	Ok(deserialized)
    	}

• from_str:
  

  fastsim/rust/fastsim-core/src/traits.rs

  Lines 106 to 112 in d0a15ae

  	fn from_str(contents: &str, format: &str) -> anyhow::Result<Self> {
  	Ok(match format {
  	"yaml" => serde_yaml::from_str(contents)?,
  	"json" => serde_json::from_str(contents)?,
  	_ => anyhow::bail!("Unsupported file format: {format:?}"),
  	})
  	}

  • reads data from a string (this did end up being the easiest way to load compiled data)
  • since this is part of the trait, it can't also accept a name (would have a different function signature), so the from_csv_str method still exists which can still do that
    • RustCycle::from_str with format="csv" can still be used if you don't care about the cycle name, it will just use the from_csv_str method with name = "":
      

      fastsim/rust/fastsim-core/src/cycle.rs

      Lines 658 to 667 in d0a15ae

      	// Note that using this method to instantiate a RustCycle from CSV instead of
      	// the `from_csv_str` method directly sets the cycle name to an empty string.
      	fn from_str(contents: &str, format: &str) -> anyhow::Result<Self> {
      	Ok(match format {
      	"yaml" => serde_yaml::from_str(contents)?,
      	"json" => serde_json::from_str(contents)?,
      	"csv" => Self::from_csv_str(contents, "")?,
      	_ => anyhow::bail!("Unsupported file format: {format:?}"),
      	})
      	}

• from_resource:
  

  fastsim/rust/fastsim-core/src/traits.rs

  Lines 78 to 96 in d0a15ae

  	fn from_resource<P: AsRef<Path>>(filepath: P) -> anyhow::Result<Self> {
  	let filepath = filepath.as_ref();
  	let contents = crate::resources::RESOURCES_DIR
  	.get_file(filepath)
  	.and_then(include_dir::File::contents_utf8)
  	.with_context(|| format!("File not found in resources: \"{}\"", filepath.display()))?;
  	let extension = filepath
  	.extension()
  	.and_then(OsStr::to_str)
  	.with_context(|| {
  	format!(
  	"File extension could not be parsed: \"{}\"",
  	filepath.display()
  	)
  	})?;
  	let mut deserialized = Self::from_str(contents, extension)?;
  	deserialized.init()?;
  	Ok(deserialized)
  	}

  • accepts a path to read from fastsim-core/resources
    • for example:
      

      fastsim/rust/fastsim-core/src/simdrivelabel.rs

      Lines 200 to 201 in d0a15ae

      	cyc.insert("udds", RustCycle::from_resource("cycles/udds.csv")?);
      	cyc.insert("hwy", RustCycle::from_resource("cycles/hwfet.csv")?);

  • uses from_str in the backend, which matches based on the filepath extension
  • this should probably be exposed to Python eventually

[kylecarow]

@calbaker I would be fine with changing the tests that return a result back to returning (), since results in tests aren't doing anything special than allowing ? in place of .unwrap()

[kylecarow]

@calbaker

I have found a lot of functions that return results, when they don't contain any fallible code. This happens in a lot of the methods called by solve_step.

For example:

This code

    /// Perform all the calculations to solve 1 time step.
    pub fn solve_step(&mut self, i: usize) -> anyhow::Result<()> {
        self.set_misc_calcs(i)?;
        self.set_comp_lims(i)?;
        self.set_power_calcs(i)?;
        self.set_ach_speed(i)?;
        self.set_hybrid_cont_calcs(i)?;
        self.set_fc_forced_state_rust(i)?;
        self.set_hybrid_cont_decisions(i)?;
        self.set_fc_power(i)?;
        Ok(())
    }

    /// Sets misc. calculations at time step 'i'
    /// Arguments:
    /// ----------
    /// i: index of time step
    pub fn set_misc_calcs(&mut self, i: usize) -> anyhow::Result<()> {
        // if cycle iteration is used, auxInKw must be re-zeroed to trigger the below if statement
        // TODO: this is probably computationally expensive and was probably a workaround for numba
        // figure out a way to not need this
        if self.aux_in_kw.slice(s![i..]).iter().all(|&x| x == 0.0) {
            // if all elements after i-1 are zero, trigger default behavior; otherwise, use override value
            if self.veh.no_elec_aux {
                self.aux_in_kw[i] = self.veh.aux_kw / self.veh.alt_eff;
            } else {
                self.aux_in_kw[i] = self.veh.aux_kw;
            }
        }
        // Is SOC below min threshold?
        self.reached_buff[i] = self.soc[i - 1] >= (self.veh.min_soc + self.veh.perc_high_acc_buf);

        // Does the engine need to be on for low SOC or high acceleration
        self.high_acc_fc_on_tag[i] = self.soc[i - 1] < self.veh.min_soc
            || (self.high_acc_fc_on_tag[i - 1] && !(self.reached_buff[i]));
        self.max_trac_mps[i] =
            self.mps_ach[i - 1] + (self.veh.max_trac_mps2 * self.cyc.dt_s_at_i(i));
        Ok(())
    }

and this code

    /// Perform all the calculations to solve 1 time step.
    pub fn solve_step(&mut self, i: usize) -> anyhow::Result<()> {
        self.set_misc_calcs(i);
        self.set_comp_lims(i)?;
        self.set_power_calcs(i)?;
        self.set_ach_speed(i)?;
        self.set_hybrid_cont_calcs(i)?;
        self.set_fc_forced_state_rust(i)?;
        self.set_hybrid_cont_decisions(i)?;
        self.set_fc_power(i)?;
        Ok(())
    }

    /// Sets misc. calculations at time step 'i'
    /// Arguments:
    /// ----------
    /// i: index of time step
    pub fn set_misc_calcs(&mut self, i: usize) {
        // if cycle iteration is used, auxInKw must be re-zeroed to trigger the below if statement
        // TODO: this is probably computationally expensive and was probably a workaround for numba
        // figure out a way to not need this
        if self.aux_in_kw.slice(s![i..]).iter().all(|&x| x == 0.0) {
            // if all elements after i-1 are zero, trigger default behavior; otherwise, use override value
            if self.veh.no_elec_aux {
                self.aux_in_kw[i] = self.veh.aux_kw / self.veh.alt_eff;
            } else {
                self.aux_in_kw[i] = self.veh.aux_kw;
            }
        }
        // Is SOC below min threshold?
        self.reached_buff[i] = self.soc[i - 1] >= (self.veh.min_soc + self.veh.perc_high_acc_buf);

        // Does the engine need to be on for low SOC or high acceleration
        self.high_acc_fc_on_tag[i] = self.soc[i - 1] < self.veh.min_soc
            || (self.high_acc_fc_on_tag[i - 1] && !(self.reached_buff[i]));
        self.max_trac_mps[i] =
            self.mps_ach[i - 1] + (self.veh.max_trac_mps2 * self.cyc.dt_s_at_i(i));
    }

Achieve exactly the same thing, and results aren't giving us any benefit

What's your opinion on removing the Result returns and Ok(())s in cases like these?

[calbaker]

What's your opinion on removing the Result returns and Ok(())s in cases like these?

@kylecarow, I'd say keep it as is! On numerous occasions, I've ended up changing functions that do not contain fallible code to contain fallible code, and returning a result that does nothing future proofs for that without imposing any meaningful computational costs, which means that if the function/method body changes, no other api changes are needed.

[kylecarow]

@calbaker Sounds reasonable to me. I marked this as ready for review, feel free to merge when tests pass and you're happy with the changes.

[kylecarow]

@calbaker didnt you say something about wanting to be able to read/write bincode objects? I could set that up to work with a .bin extension (or something) really easily in this branch

[kylecarow]

@calbaker feel free to merge when you're happy with the changes, i added an approving review to let the merge happen

[kylecarow]

@calbaker I used .trim_start_matches('.') on strings to ignore leading .s, which simplifies the match conditions a bit

fastsim/rust/fastsim-core/src/traits.rs

Lines 110 to 115 in b8bdfa2

	match format.trim_start_matches('.').to_lowercase().as_str() {
	"yaml" | "yml" => Self::from_yaml(contents),
	"json" => Self::from_json(contents),
	_ => bail!(
	"Unsupported file format {format:?}, must be one of {ACCEPTED_FILE_FORMATS:?}"
	),

[kylecarow]

I added an implementation of to_file and to_str for RustCycle, so we can now write cycle CSV files and strings from Rust 😁

[kylecarow]

I'll go ahead and merge this into fastsim-2, with the understanding that bincode serde is broken for vehicles and simdrives (as it currently is in fastsim-2 anyway)

see https://github.nrel.gov/MBAP/fastsim/issues/279 for details