Skip to content

Commit 349dd61

Browse files
authored
feat: track package descriptions when loading (#2639)
# Goals - Track descriptions during package loading such that partial descriptions can be provided even if an error is returned - Provide a `describe` cli subcommand for high-level descriptions of a package, primarily for debug purposes #2650. Since we want a description even if some loading stage fails (e.g. extension resolution), tracking partial descriptions is a requirement. ## Secondary goals - Unify loading approaches across json/model, centralise more validation logic - Reduce burden on EnvelopeError - more local errors - Replace existing debug extraction (generator, used extensions, envelopeconfig) with single description # Approach In order to make this PR non-breaking I have deprecated existing reading functions, and introduced new functions that return a full description on successful read, and a partial when error. The deprecated functions are most of the missing test coverage. Reading naturally falls in to two stages: header and payload. If the header fails no description is valid, so that error does not come with a description. Payload errors can come with descriptions. In order to build descriptions while loading I introduce the `Reader` struct to track data across the reading stages. This is handed off to the model `Context` when that stage takes over. ## Partial descriptions Descriptions are defined in description.rs. They are intended to be small amounts of simple data, so all fields are pub. All fields except the envelope header are optional, a value of None indicates the field has not been set yet. ## Appendix: JSON Since the JSON format is effectively legacy I did not focus much on incremental description tracking, it is more difficult since serde likes to read all at once. I found a surprise performance regression in JSON loading. In commit [af01043](af01043) I attempted to address this by removing an unnecessary two stage serde deser, which has swung things in the other direction, improving performance from prior. ## Appendix: EnvelopeError EnvelopeError is used as a catch all for all reading and writing errors. This PR goes some way to introducing new more specific errors for reaeding. EnvelopeError should really in future have the reading errors be taken out of it and renamed to `WriteError` or similar, then all the conversions from the reading errors can be removed. Removing the deprecated read functions is a pre-requisite for this. The merging of this PR should be accompanied by issues for these follow ups.
1 parent a1ce622 commit 349dd61

File tree

17 files changed

+1307
-297
lines changed

17 files changed

+1307
-297
lines changed

hugr-cli/src/convert.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -49,7 +49,7 @@ pub struct ConvertArgs {
4949
impl ConvertArgs {
5050
/// Convert a HUGR between different envelope formats
5151
pub fn run_convert(&mut self) -> Result<()> {
52-
let (env_config, package) = self.input_args.get_envelope()?;
52+
let (env_config, package) = self.input_args.get_described_package()?;
5353

5454
// Handle text and binary format flags, which override the format option
5555
let mut config = if self.text {
@@ -67,7 +67,7 @@ impl ConvertArgs {
6767
"model-text-exts" => EnvelopeFormat::ModelTextWithExtensions,
6868
_ => Err(CliError::InvalidFormat(fmt.clone()))?,
6969
},
70-
None => env_config.format, // Use input format if not specified
70+
None => env_config.header.config().format, // Use input format if not specified
7171
};
7272
EnvelopeConfig::new(format)
7373
};

hugr-cli/src/hugr_io.rs

Lines changed: 19 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,8 @@
11
//! Input/output arguments for the HUGR CLI.
22
33
use clio::Input;
4-
use hugr::envelope::{EnvelopeConfig, EnvelopeError, read_envelope};
4+
use hugr::envelope::description::PackageDesc;
5+
use hugr::envelope::{EnvelopeConfig, read_described_envelope};
56
use hugr::extension::ExtensionRegistry;
67
use hugr::package::Package;
78
use hugr::{Extension, Hugr};
@@ -29,7 +30,7 @@ pub struct HugrInputArgs {
2930
short,
3031
long,
3132
help_heading = "Input",
32-
help = "Paths to serialised extensions to validate against."
33+
help = "Paths to additional serialised extensions needed to load the Hugr."
3334
)]
3435
pub extensions: Vec<PathBuf>,
3536
/// Read the input as a HUGR JSON file instead of an envelope.
@@ -48,7 +49,7 @@ impl HugrInputArgs {
4849
/// If [`HugrInputArgs::hugr_json`] is `true`, [`HugrInputArgs::get_hugr`] should be called instead as
4950
/// reading the input as a package will fail.
5051
pub fn get_package(&mut self) -> Result<Package, CliError> {
51-
self.get_envelope().map(|(_, package)| package)
52+
self.get_described_package().map(|(_, package)| package)
5253
}
5354

5455
/// Read a hugr envelope from the input and return the envelope
@@ -58,13 +59,24 @@ impl HugrInputArgs {
5859
///
5960
/// If [`HugrInputArgs::hugr_json`] is `true`, [`HugrInputArgs::get_hugr`] should be called instead as
6061
/// reading the input as a package will fail.
62+
#[deprecated(since = "0.24.1", note = "Use get_described_envelope instead")]
6163
pub fn get_envelope(&mut self) -> Result<(EnvelopeConfig, Package), CliError> {
64+
let (desc, package) = self.get_described_package()?;
65+
Ok((desc.header.config(), package))
66+
}
67+
68+
/// Read a hugr envelope from the input and return the envelope
69+
/// description and the decoded package.
70+
///
71+
/// # Errors
72+
///
73+
/// If [`HugrInputArgs::hugr_json`] is `true`, [`HugrInputArgs::get_hugr`] should be called instead as
74+
/// reading the input as a package will fail.
75+
pub fn get_described_package(&mut self) -> Result<(PackageDesc, Package), CliError> {
6276
let extensions = self.load_extensions()?;
6377
let buffer = BufReader::new(&mut self.input);
64-
read_envelope(buffer, &extensions).map_err(|e| match e {
65-
EnvelopeError::MagicNumber { .. } => CliError::NotAnEnvelope,
66-
_ => CliError::Envelope(e),
67-
})
78+
79+
Ok(read_described_envelope(buffer, &extensions)?)
6880
}
6981
/// Read a hugr JSON file from the input.
7082
///

hugr-cli/src/lib.rs

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -98,6 +98,9 @@ pub enum CliError {
9898
/// The generator of the HUGR.
9999
generator: Box<String>,
100100
},
101+
#[error("Error reading envelope.")]
102+
/// Errors produced when reading an envelope.
103+
ReadEnvelope(#[from] hugr::envelope::ReadError),
101104
}
102105

103106
impl CliError {

hugr-cli/src/mermaid.rs

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -43,9 +43,8 @@ impl MermaidArgs {
4343

4444
/// Write the mermaid diagram for a HUGR envelope.
4545
pub fn run_print_envelope(&mut self) -> Result<()> {
46-
let package = self.input_args.get_package()?;
47-
let generator = hugr::envelope::get_generator(&package.modules);
48-
46+
let (desc, package) = self.input_args.get_described_package()?;
47+
let generator = desc.generator();
4948
if self.validate {
5049
package
5150
.validate()

hugr-cli/src/validate.rs

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -30,14 +30,15 @@ impl ValArgs {
3030
if self.input_args.hugr_json {
3131
#[allow(deprecated)]
3232
let hugr = self.input_args.get_hugr()?;
33+
#[allow(deprecated)]
3334
let generator = hugr::envelope::get_generator(&[&hugr]);
3435

3536
hugr.validate()
3637
.map_err(PackageValidationError::Validation)
3738
.map_err(|val_err| CliError::validation(generator, val_err))?;
3839
} else {
39-
let package = self.input_args.get_package()?;
40-
let generator = hugr::envelope::get_generator(&package.modules);
40+
let (desc, package) = self.input_args.get_described_package()?;
41+
let generator = desc.generator();
4142
package
4243
.validate()
4344
.map_err(|val_err| CliError::validation(generator, val_err))?;

hugr-cli/tests/convert.rs

Lines changed: 18 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@
77
use assert_cmd::Command;
88
use assert_fs::{NamedTempFile, fixture::FileWriteStr};
99
use hugr::builder::{DataflowSubContainer, ModuleBuilder};
10-
use hugr::envelope::{EnvelopeConfig, EnvelopeFormat, read_envelope};
10+
use hugr::envelope::{EnvelopeConfig, EnvelopeFormat, read_described_envelope};
1111
use hugr::package::Package;
1212
use hugr::types::Type;
1313
use hugr::{
@@ -77,7 +77,9 @@ fn test_convert_to_json(test_envelope_file: NamedTempFile, mut convert_cmd: Comm
7777
let output_content = std::fs::read(output_file.path()).expect("Failed to read output file");
7878
let reader = BufReader::new(output_content.as_slice());
7979
let registry = ExtensionRegistry::default();
80-
let (config, _) = read_envelope(reader, &registry).expect("Failed to read output envelope");
80+
let (desc, _) =
81+
read_described_envelope(reader, &registry).expect("Failed to read output envelope");
82+
let config = desc.header.config();
8183

8284
// Verify the format is correct
8385
assert_eq!(config.format, EnvelopeFormat::PackageJson);
@@ -102,8 +104,9 @@ fn test_convert_to_model(test_envelope_file: NamedTempFile, mut convert_cmd: Com
102104
let output_content = std::fs::read(output_file.path()).expect("Failed to read output file");
103105
let reader = BufReader::new(output_content.as_slice());
104106
let registry = ExtensionRegistry::default();
105-
let (config, _) = read_envelope(reader, &registry).expect("Failed to read output envelope");
106-
107+
let (desc, _) =
108+
read_described_envelope(reader, &registry).expect("Failed to read output envelope");
109+
let config = desc.header.config();
107110
// Verify the format is correct
108111
assert_eq!(config.format, EnvelopeFormat::Model);
109112
}
@@ -174,7 +177,9 @@ fn test_convert_model_text_format(test_envelope_file: NamedTempFile, mut convert
174177
let output_content = std::fs::read(output_file.path()).expect("Failed to read output file");
175178
let reader = BufReader::new(output_content.as_slice());
176179
let registry = ExtensionRegistry::default();
177-
let (config, _) = read_envelope(reader, &registry).expect("Failed to read output envelope");
180+
let (desc, _) =
181+
read_described_envelope(reader, &registry).expect("Failed to read output envelope");
182+
let config = desc.header.config();
178183

179184
// Verify the format is correct
180185
assert_eq!(config.format, EnvelopeFormat::ModelText);
@@ -192,14 +197,14 @@ fn test_format_roundtrip(test_package: Package) {
192197
let config_model = EnvelopeConfig::new(EnvelopeFormat::Model);
193198
let reader = BufReader::new(json_data.as_slice());
194199
let registry = ExtensionRegistry::default();
195-
let (_, package) = read_envelope(reader, &registry).unwrap();
200+
let (_, package) = read_described_envelope(reader, &registry).unwrap();
196201

197202
let mut model_data = Vec::new();
198203
hugr::envelope::write_envelope(&mut model_data, &package, config_model).unwrap();
199204

200205
// Convert back to JSON
201206
let reader = BufReader::new(model_data.as_slice());
202-
let (_, package_back) = read_envelope(reader, &registry).unwrap();
207+
let (_, package_back) = read_described_envelope(reader, &registry).unwrap();
203208

204209
// Package should be the same after roundtrip conversion
205210
assert_eq!(test_package, package_back);
@@ -215,7 +220,9 @@ fn test_convert_text_flag(test_envelope_text: (String, Package), mut convert_cmd
215220

216221
let reader = BufReader::new(stdout.as_slice());
217222
let registry = ExtensionRegistry::default();
218-
let (config, _) = read_envelope(reader, &registry).expect("Failed to read output envelope");
223+
let (desc, _) =
224+
read_described_envelope(reader, &registry).expect("Failed to read output envelope");
225+
let config = desc.header.config();
219226

220227
// Verify it's a text-based format
221228
assert!(config.format.ascii_printable());
@@ -231,7 +238,9 @@ fn test_convert_binary_flag(test_envelope_text: (String, Package), mut convert_c
231238

232239
let reader = BufReader::new(stdout.as_slice());
233240
let registry = ExtensionRegistry::default();
234-
let (config, _) = read_envelope(reader, &registry).expect("Failed to read output envelope");
241+
let (desc, _) =
242+
read_described_envelope(reader, &registry).expect("Failed to read output envelope");
243+
let config = desc.header.config();
235244

236245
// Verify it's a binary format (not ASCII printable)
237246
assert!(!config.format.ascii_printable());

hugr-cli/tests/validate.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -151,7 +151,7 @@ fn test_bad_json(mut val_cmd: Command) {
151151
val_cmd
152152
.assert()
153153
.failure()
154-
.stderr(contains("Error decoding HUGR envelope"))
154+
.stderr(contains("Error reading package payload in envelope"))
155155
.stderr(contains("missing field"));
156156
}
157157

0 commit comments

Comments
 (0)