-
Notifications
You must be signed in to change notification settings - Fork 151
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
[POC] Dashboard generator #522
Conversation
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
Nitpick: the output of the progress bar get overwritten to produce:
Maybe we can avoid that? |
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 is how far I got until now - looking really great!
for more information, see https://pre-commit.ci
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.
⭐ 🚀 ❤️ Incredible work - really love is !!!
I am almost through, basically just the Layout to go - but approving already now as I think this is in very good shape already!! Well done, you should be very proud!
component_id: str = Field( | ||
pattern=r"^[a-z]+(_[a-z]+)?$", description="Small snake case description of this component." | ||
) | ||
df_name: str = Field( |
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.
Should this not have a validator that verifies this DF is even present?
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.
Good point. Although this means we would need to create ComponentPlan at run time too, so the validator can access the "all_df_metadata". Something like this:
@root_validator(allow_reuse=True)
def _check_df_name(cls, values):
df_name = values.get("df_name")
component_id = values.get("component_id")
component_description = values.get("component_description")
available_df_names = all_df_metadata.get_df_names()
if df_name not in available_df_names:
logger.warning(
f"""
[FALLBACK] Failed to build `Component`: {component_id}.
Reason: Dataframe `{df_name}` not found in metadata df names `{available_df_names}`.
Relevant prompt: {component_description}
"""
)
return {"component_type": "Card", "component_id": component_id, "component_description": f"Failed to build component: {component_id}", "df_name": "N/A"}
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.
Leave as is for now, as it would be a slightly big refactor. Let's discuss further before taking actions
try: | ||
page = vm.Page(title=title, components=components, controls=controls, layout=layout) | ||
except Exception as e: | ||
if any("Number of page and grid components need to be the same" in error["msg"] for error in e.errors()): |
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.
Can we not catch this earlier, when the layout is created?
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.
Good question. I created the ticket for next sprint to revisit this. Ideally we should return no layout if failed.
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.
🥳 Awesome, took a look at the last bits - I think I cannot find anything for the moment any more - looking forward to the merge!!
return grid | ||
|
||
|
||
class LayoutPlan(BaseModel): |
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.
Could we not enhance this model with validators? Basically we can check if the number of components etc is correct?
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.
Yes, replied in an earlier thread. Will revisit in next sprint 🙌
raise ValidationError(f"Component ids must be unique. Duplicated component ids: {duplicates}") | ||
return values | ||
|
||
def __init__(self, **data): |
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.
Why do we need this custom __init__
?
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.
to make those 3 private attributes visible, otherwise got attribute error
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.
Hm let's revisit this
Description
Run
or run
Remaining TODOs:
Screenshot
Notice
I acknowledge and agree that, by checking this box and clicking "Submit Pull Request":