-
Notifications
You must be signed in to change notification settings - Fork 320
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
Reimplement attributes view for component debug screen #3475
Conversation
caff78d
to
a4989ff
Compare
bors try |
tryBuild failed: |
a4989ff
to
8271b39
Compare
bors try |
bors cancel |
8271b39
to
0e135b5
Compare
tryBuild failed: |
bors try |
tryBuild succeeded: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright @britmyerss, you wanted nits... you got nits!
@@ -1,7 +1,7 @@ | |||
<template> | |||
<div class="overflow-hidden my-xs p-xs border-opacity-10 border-l-2"> | |||
<dl class="flex flex-col gap-xs"> | |||
<DebugViewItem title="Attribute Value Id" :data="data.valueId" /> | |||
<DebugViewItem title="Attribute Value Id" :data="data.attributeValueId" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question about the name of this file. Is it an AttributeValue
view, an AttributePrototype
view, both or something else?
We're overloading the usage of the word "Attribute" across the system to mean subtly different things and should tighten it up. However, the whole world is unlikely in scope for this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah I asked myself the same thing here. I decided to keep it as is both because the whole world is not in scope for this, and because this actually is a debug view of the attributes and we're displaying the AttributePrototype
info from the perspective of the AttributeValues
- but your questions below, I separated this on the backend because the next step is to bring back input/output sockets, which will also display the same AttributePrototype
debug data for them AND I want to surface this on the viz diagram eventually.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay that sounds good to me.
@@ -33,6 +33,7 @@ use crate::{ | |||
}; | |||
|
|||
pub mod argument; | |||
pub mod debug; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the module!
use std::collections::HashMap; | ||
|
||
use serde::{Deserialize, Serialize}; | ||
|
||
use thiserror::Error; | ||
|
||
use crate::attribute::prototype::{ | ||
argument::{ | ||
static_value::StaticArgumentValue, | ||
value_source::{ValueSource, ValueSourceError}, | ||
AttributePrototypeArgument, AttributePrototypeArgumentError, | ||
}, | ||
AttributePrototypeError, | ||
}; | ||
use crate::attribute::value::{AttributeValueError, ValueIsFor}; | ||
|
||
use crate::prop::{PropError, PropPath}; | ||
|
||
use crate::func::execution::{FuncExecution, FuncExecutionError}; | ||
use crate::socket::input::InputSocketError; | ||
use crate::socket::output::OutputSocketError; | ||
use crate::workspace_snapshot::node_weight::NodeWeightError; | ||
use crate::workspace_snapshot::WorkspaceSnapshotError; | ||
use crate::{ | ||
AttributePrototype, AttributePrototypeId, AttributeValue, AttributeValueId, Component, | ||
ComponentError, DalContext, Func, FuncError, FuncId, InputSocket, Prop, | ||
}; | ||
use serde_json::Value; | ||
use telemetry::prelude::*; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should clean up these imports. Let's put all use <non-crate>
statements at the top, then a blank line, then all use crate
statements.
Some folks split use std
statements in their files too, but we usually group them together (unclear if by habit or intention, but that's a topic for another day).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't have a linter that will sort those for us?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IDEs typically do. There are a few features needed to do this well via automation and at least one of them is not stable
yet: rust-lang/rustfmt#4991.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will do! yeah my IDE does some of this formatting and merging for me but I wasn't sure what 'more right' vs 'less right' looked like :)
|
||
#[derive(Clone, Debug, Serialize, Deserialize)] | ||
#[serde(rename_all = "camelCase")] | ||
pub struct AttributePrototypeDebugView { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this answers my other question.
other_source => { | ||
let mut values = vec![]; | ||
|
||
for av_id in other_source | ||
.attribute_values_for_component_id( | ||
ctx, | ||
expected_source_component_id, | ||
) | ||
.await? | ||
{ | ||
let attribute_value = AttributeValue::get_by_id(ctx, av_id).await?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A smell for me when match arms get too big is when you have a scope that contains non-trivial iterator or recursive logic in them. I'd look into breaking this up.
Example:
match foo {
Some(v) => println!("{v}"),
None => {
for i in big_ass_vec {
// a bunch of code
}
lib/dal/src/attribute/value.rs
Outdated
#[instrument(level = "info", skip_all)] | ||
/// Get the morale equivalent of the prop path for a given attribute value id. | ||
/// This includes the key/index in the path, unlike the path for a Prop which doesn't | ||
/// include the key/index | ||
pub async fn get_path_for_id( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For style, I'd put the comment above the macro.
lib/dal/src/attribute/value.rs
Outdated
let prop_name = match AttributeValue::prop_id_for_id(ctx, attribute_value_id).await { | ||
Ok(prop) => Prop::get_by_id(ctx, prop).await?.name, | ||
Err(_) => { | ||
// This means it's an input or output socket, which has no path and should just be empty string | ||
continue; | ||
} | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the only error possible here the one that you listed? If not, then we should handle the specific error or refactor the prop_id_for_id
function to return a different type to help you determine that (e.g. Result<Option<T>, E>>
instead of Result<T, E>
).
lib/dal/src/attribute/value.rs
Outdated
let order_of_thing_node = ctx | ||
.workspace_snapshot()? | ||
.incoming_sources_for_edge_weight_kind( | ||
attribute_value_id, | ||
EdgeWeightKindDiscriminants::Ordinal, | ||
) | ||
.await? | ||
.pop() | ||
.ok_or(AttributeValueError::NoOrderingNodeForAttributeValue( | ||
attribute_value_id, | ||
))?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be its own function for AttributeValue
. We do not want to expose the snapshot.
lib/dal/src/attribute/value.rs
Outdated
let order_node_weight = ctx | ||
.workspace_snapshot()? | ||
.get_node_weight(order_of_thing_node) | ||
.await? | ||
.get_ordering_node_weight()?; | ||
|
||
let order = order_node_weight.order(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here as above.... Maybe on WorkspaceSnapshot
itself though since "ordering nodes" do not belong to SI-specific entities.
lib/dal/src/attribute/value.rs
Outdated
let name = format!("{0}[{1}]", prop_name, &index.to_string()); | ||
parts.push_front(name.to_owned()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is being assembled here? It looks like the path for display purposes in the frontend.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is correct. This function is supposed to assemble a path to the attribute value, for display in the debugging panel. So if its an array item, it would be something like /root/domain/things/thing[0]
with 0
being the index of the item, thing
being the prop name for entry and things
the prop name for the array.
For a map its the same thing but with key instead of index
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gotcha. Is there a reason why we wouldn't use PropPath
instead of strings? Is it because it doesn't work with array items?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exactly. Prop paths are different from the attribute value paths this creates since we show keys and indices
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see!! String manipulation makes me nervous... I wonder if a theoretical AttributeValuePath
entity would be useful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I debated making an AttributeValuePath
entity, because I was honestly surprised we didn't have this yet. But it felt strange to formalize it for just this debug view and I figured if we end up needing it later we can always formalize it then and replace this implementation with that one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I won't be a blocker here, but I would personally formalize it because there may be a day 2 bug or feature extension here that relies on using/changing/extending the string manipulation.
It's a nit though.
lib/dal/src/attribute/value.rs
Outdated
let edge_weight = ctx | ||
.workspace_snapshot()? | ||
.edges_directed_for_edge_weight_kind( | ||
attribute_value_id, | ||
Direction::Incoming, | ||
EdgeWeightKindDiscriminants::Contain, | ||
) | ||
.await? | ||
.pop() | ||
.ok_or(AttributeValueError::ParentAttributeValueMissing( | ||
attribute_value_id, | ||
))?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment as above with not exposing the graph
lib/dal/src/attribute/value/debug.rs
Outdated
use std::collections::HashMap; | ||
|
||
use serde::{Deserialize, Serialize}; | ||
|
||
use thiserror::Error; | ||
|
||
use crate::attribute::prototype::debug::{ | ||
AttributePrototypeDebugView, AttributePrototypeDebugViewError, | ||
}; | ||
use crate::attribute::prototype::{ | ||
argument::{value_source::ValueSourceError, AttributePrototypeArgumentError}, | ||
AttributePrototypeError, | ||
}; | ||
use crate::attribute::value::AttributeValueError; | ||
use crate::func::execution::FuncExecutionError; | ||
|
||
use crate::prop::PropError; | ||
|
||
use crate::socket::input::InputSocketError; | ||
use crate::socket::output::OutputSocketError; | ||
|
||
use crate::workspace_snapshot::node_weight::NodeWeightError; | ||
use crate::workspace_snapshot::WorkspaceSnapshotError; | ||
|
||
use crate::ComponentError; | ||
use telemetry::prelude::*; | ||
|
||
use crate::{func::execution::FuncExecution, AttributeValue, AttributeValueId, DalContext, Prop}; | ||
use crate::{AttributePrototypeId, FuncError, FuncId, PropKind}; | ||
|
||
use super::ValueIsFor; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment about imports as before
lib/dal/src/attribute/value/debug.rs
Outdated
#[error(transparent)] | ||
AttributePrototypeArgumentError(#[from] AttributePrototypeArgumentError), | ||
#[error(transparent)] | ||
AttributePrototypeDebugViewError(#[from] AttributePrototypeDebugViewError), | ||
#[error(transparent)] | ||
AttributePrototypeError(#[from] AttributePrototypeError), | ||
#[error(transparent)] | ||
AttributeValue(#[from] AttributeValueError), | ||
#[error(transparent)] | ||
ComponentError(#[from] ComponentError), | ||
#[error(transparent)] | ||
Func(#[from] FuncError), | ||
#[error(transparent)] | ||
FuncExecution(#[from] FuncExecutionError), | ||
#[error(transparent)] | ||
InputSocketError(#[from] InputSocketError), | ||
#[error(transparent)] | ||
NodeWeightError(#[from] NodeWeightError), | ||
#[error(transparent)] | ||
OutputSocketError(#[from] OutputSocketError), | ||
#[error(transparent)] | ||
PropError(#[from] PropError), | ||
#[error(transparent)] | ||
ValueSourceError(#[from] ValueSourceError), | ||
#[error(transparent)] | ||
WorkspaceSnapshotError(#[from] WorkspaceSnapshotError), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment about transparent
as before
lib/dal/src/attribute/value/debug.rs
Outdated
let attribute_value = AttributeValue::get_by_id(ctx, attribute_value_id).await?; | ||
let prototype_id = AttributeValue::prototype_id(ctx, attribute_value_id).await?; | ||
let value_is_for = AttributeValue::is_for(ctx, attribute_value_id).await?; | ||
|
||
let prop_id = AttributeValue::prop_id_for_id(ctx, attribute_value_id).await?; | ||
|
||
let prop = Prop::get_by_id(ctx, prop_id).await?; | ||
let path = match AttributeValue::get_path_for_id(ctx, attribute_value_id).await? { | ||
Some(path) => path, | ||
None => String::new(), | ||
}; | ||
let prop_opt: Option<Prop> = Some(prop); | ||
let attribute_prototype_debug_view = | ||
AttributePrototypeDebugView::new_from_attribute_value(ctx, attribute_value_id).await?; | ||
let materialized_view = attribute_value.materialized_view(ctx).await?; | ||
let prop_kind = prop_opt.clone().map(|prop| prop.kind); | ||
let value = match attribute_value.unprocessed_value(ctx).await? { | ||
Some(value) => Some(value), | ||
None => attribute_value.value(ctx).await?, | ||
}; | ||
let view = AttributeDebugView { | ||
path, | ||
parent_id, | ||
attribute_value_id, | ||
func_id: attribute_prototype_debug_view.func_id, | ||
key, | ||
func_execution: attribute_prototype_debug_view.func_execution, | ||
prop: prop_opt, | ||
prototype_id: Some(prototype_id), | ||
value_is_for, | ||
func_name: attribute_prototype_debug_view.func_name, | ||
func_args: attribute_prototype_debug_view.func_args, | ||
arg_sources: attribute_prototype_debug_view.arg_sources, | ||
value, | ||
prop_kind, | ||
materialized_view, | ||
}; | ||
Ok(view) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now I'm a bit confused. What's the difference between this and the other view?
lib/dal/src/component/debug.rs
Outdated
pub attribute_tree: HashMap<AttributeValueId, Vec<AttributeValueId>>, | ||
// pub input_sockets: Vec<AttributeDebugView>, | ||
// pub output_sockets: Vec<AttributeDebugView>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should delete the commented out stuff unless accompanied by a NOTE
, TODO
or FIXME
.
lib/dal/src/component/debug.rs
Outdated
#[remain::sorted] | ||
#[derive(Error, Debug)] | ||
pub enum ComponentDebugViewError { | ||
#[error(transparent)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment about transparent
lib/dal/src/component/debug.rs
Outdated
} | ||
#[instrument(level = "info", skip_all)] | ||
pub async fn get_attribute_value_tree_for_component( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd leave a space here.
lib/dal/src/component/debug.rs
Outdated
impl ComponentDebugView { | ||
#[instrument(level = "info", skip_all)] | ||
pub async fn new(ctx: &DalContext, component: &Component) -> ComponentDebugViewResult<Self> { | ||
let schema_variant = Component::schema_variant(component, ctx).await?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: You don't need the whole schema variant. We may be able to just fetch the id from the node.
let maybe_ordering = | ||
AttributeValue::get_child_av_ids_for_ordered_parent(ctx, attribute_value_id) | ||
.await | ||
.ok(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we eating a potential error here?
let mut child_values = HashMap::new(); | ||
// Get the root attribute value and load it into the work queue. | ||
let root_attribute_value_id = Component::root_attribute_value_id(ctx, component_id).await?; | ||
let workspace_snapshot = ctx.workspace_snapshot()?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit conflicted. I see other AttributeValue
methods called in here, so that's a sign to create more methods to sandbox the snapshot. However, it makes more sense in this function than before because this is more of a raw usage scenario. I'd still investigate breaking them up.
use pretty_assertions_sorted::assert_eq; | ||
|
||
#[test] | ||
async fn get_debug_view(ctx: &mut DalContext) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's leave some spaces and comments. This is difficult to read.
Usually, the convention we have to group things by commit
when in doubt. Additionally, grouping by "preparing the environment" vs. performing mutations and assertions can be helpful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yay! Testing!
use std::time::Instant; | ||
|
||
use axum::extract::Query; | ||
use axum::Json; | ||
|
||
use serde::{Deserialize, Serialize}; | ||
|
||
use super::ComponentResult; | ||
use crate::server::extract::{AccessBuilder, HandlerContext}; | ||
use dal::{Component, ComponentId, SchemaVariantId, Visibility}; | ||
use dal::attribute::value::debug::AttributeDebugView; | ||
use dal::component::debug::ComponentDebugView; | ||
use dal::{AttributeValueId, Component, ComponentId, SchemaVariantId, Visibility}; | ||
use telemetry::prelude::*; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment as before
//grab input socket info next | ||
//grab output socket info next | ||
|
||
dbg!(transform_start.elapsed()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's delete these or put this behind an info
or debug
statement with context for what it's benchmarking.
let debug_view = ComponentDebugView::new(&ctx, &component).await?; | ||
|
||
let attributes = vec![]; | ||
let mut attributes = vec![]; | ||
let input_sockets = vec![]; | ||
let output_sockets = vec![]; | ||
|
||
// let transform_start = Instant::now(); | ||
// | ||
// for attribute_debug in debug_view.attributes { | ||
// attributes.push(FrontendAttributeDebugView { | ||
// name: attribute_debug | ||
// .prop | ||
// .as_ref() | ||
// .map(|p| p.name()) | ||
// .unwrap_or("") | ||
// .into(), | ||
// path: attribute_debug.path.to_owned(), | ||
// debug_data: get_attribute_metadata(&ctx, attribute_debug).await?, | ||
// }); | ||
// } | ||
// | ||
// for attribute_debug in debug_view.input_sockets { | ||
// input_sockets.push(FrontendAttributeDebugView { | ||
// name: attribute_debug.path.to_owned(), | ||
// path: "Input Socket".into(), | ||
// debug_data: get_attribute_metadata(&ctx, attribute_debug).await?, | ||
// }); | ||
// } | ||
// | ||
// for attribute_debug in debug_view.output_sockets { | ||
// output_sockets.push(FrontendAttributeDebugView { | ||
// name: attribute_debug.path.to_owned(), | ||
// path: "Output Socket".into(), | ||
// debug_data: get_attribute_metadata(&ctx, attribute_debug).await?, | ||
// }); | ||
// } | ||
// | ||
// dbg!(transform_start.elapsed()); | ||
let transform_start = Instant::now(); | ||
let mut cache: Vec<AttributeValueId> = vec![]; | ||
//grab attribute value debug views | ||
for (av, children) in debug_view.attribute_tree { | ||
if !cache.contains(&av) { | ||
let avd = AttributeDebugView::new(&ctx, av, None, None).await?; | ||
if let Some(name) = avd.prop.as_ref().map(|p| p.name.to_owned()) { | ||
attributes.push(FrontendAttributeDebugView { | ||
name, | ||
path: avd.path.to_owned(), | ||
debug_data: avd, | ||
}); | ||
cache.push(av); | ||
} | ||
} | ||
|
||
for child_av in children { | ||
if !cache.contains(&child_av) { | ||
let child_avd = AttributeDebugView::new(&ctx, child_av, None, Some(av)).await?; | ||
if let Some(name) = child_avd.prop.as_ref().map(|p| p.name.to_owned()) { | ||
attributes.push(FrontendAttributeDebugView { | ||
name, | ||
path: child_avd.path.to_owned(), | ||
debug_data: child_avd, | ||
}); | ||
cache.push(child_av); | ||
} | ||
} | ||
} | ||
} | ||
attributes.sort_by_key(|view| view.path.to_lowercase()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My instinct is to put this behind a dal struct and then test that in integration tests. We want sdf
routes thin in lieu of not having API-driven testing infrastructure. Up to you though if you think this is covered in tests.
0e135b5
to
0cb9e87
Compare
0cb9e87
to
ec9fe2c
Compare
bors try |
tryMerge conflict. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These changes are excellent! Love the use of the constant for the separator.
let attribute_path = AttributeValue::get_path_for_id(ctx, rigid_designator_value_id) | ||
.await | ||
.expect("can't get the path"); | ||
assert_eq!( | ||
attribute_path, | ||
Some(rigid_prop_path.with_replaced_sep("/").to_string()) | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love it!
Really awesome work @britmyerss 🥳 |
ec9fe2c
to
475c755
Compare
bors try |
tryBuild succeeded: |
bors merge |
Build succeeded: |
This PR brings back the Debug view in the Attribute Panel, with some cleanups along the way:
debug
modules for the various Node types on the graph: Socket, Attribute Prototype, Attribute Value, ComponentAttributeValuePath
struct to consolidate path creation for attribute values for props and sockets (including the key/index for child attribute values of maps/arrays)