Skip to content
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

[Feat] Refactor of vizro_ai.plot (breaking) #646

Merged
merged 82 commits into from
Sep 20, 2024

Conversation

maxschulz-COL
Copy link
Contributor

@maxschulz-COL maxschulz-COL commented Aug 22, 2024

Description

Full refactor of vizro-ai.plot. For reviewers (before docs are online):

New API (no more explain, but added validate_code):

df: pd.DataFrame,
user_input: str,
max_debug_retry: int = 3,
return_elements: bool = False,
validate_code: bool = True

Cases are below:

return_elements = False - return Vizro chart, but no explanation, or code explanation

return_elements = True - return response pydantic model. This model has the following properties and methods:

  • response.code - get dash code
  • response.code_vizro - get Vizro code
  • response.get_fig_object(self, data_frame: pd.DataFrame, chart_name: Optional[str] = None, vizro=True)
    --> re-execute code to obtain fig object with DF of choice, with a different chart name, and as Vizro or plotly chart

validate_code: bool = False - does not execute any code, but code string is not validated by actually running it

Screenshot

Notice

  • I acknowledge and agree that, by checking this box and clicking "Submit Pull Request":

    • I submit this contribution under the Apache 2.0 license and represent that I am entitled to do so on behalf of myself, my employer, or relevant third parties, as applicable.
    • I certify that (a) this contribution is my original creation and / or (b) to the extent it is not my original creation, I am authorized to submit this contribution on behalf of the original creator(s) or their licensees.
    • I certify that the use of this contribution as authorized by the Apache 2.0 license does not violate the intellectual property rights of anyone else.
    • I have not referenced individuals, products or companies in any commits, directly or indirectly.
    • I have not added data or restricted code in any commits, directly or indirectly.

@maxschulz-COL maxschulz-COL added the Vizro-AI 🤖 Issue/PR that addresses Vizro-AI package label Aug 22, 2024
Copy link
Contributor

@lingyielia lingyielia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very excited to seeing this PR now shaping up!

vizro-ai/src/vizro_ai/_vizro_ai.py Outdated Show resolved Hide resolved
vizro-ai/src/vizro_ai/_vizro_ai.py Outdated Show resolved Hide resolved
vizro-ai/src/vizro_ai/_vizro_ai.py Outdated Show resolved Hide resolved
vizro-ai/src/vizro_ai/plot2/_response_models.py Outdated Show resolved Hide resolved
@maxschulz-COL maxschulz-COL changed the title [DRAFT] Refactor of vizro_ai.charts [DRAFT] Refactor of vizro_ai.plot Aug 23, 2024
Copy link
Contributor

@nadijagraca nadijagraca left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks really good. I love seeing so much code deleted, so much cleaner. 🧹 🙌

I did encounter a small issue with safeguard code validation when trying to run an example. In the plot description I asked for a horizontal line to be added to the chart (the error message is below).

I'm not sure if this is intended behaviour or not.
Screenshot 2024-09-05 at 16 02 54

vizro-ai/src/vizro_ai/_vizro_ai.py Outdated Show resolved Hide resolved
@lingyielia lingyielia self-requested a review September 5, 2024 15:17
@maxschulz-COL
Copy link
Contributor Author

Looks really good. I love seeing so much code deleted, so much cleaner. 🧹 🙌

I did encounter a small issue with safeguard code validation when trying to run an example. In the plot description I asked for a horizontal line to be added to the chart (the error message is below).

I'm not sure if this is intended behaviour or not. Screenshot 2024-09-05 at 16 02 54

Thanks for reporting this - strange, I could not really tell which built-in this is, surely len must be allowed haha. But I will check

@lingyielia
Copy link
Contributor

Looks really good. I love seeing so much code deleted, so much cleaner. 🧹 🙌
I did encounter a small issue with safeguard code validation when trying to run an example. In the plot description I asked for a horizontal line to be added to the chart (the error message is below).
I'm not sure if this is intended behaviour or not.

Thanks for reporting this - strange, I could not really tell which built-in this is, surely len must be allowed haha. But I will check

Ah it's caused by type, which is not a whitelist word

raise Exception(
            f"Unsafe builtin functions {builtin_str} are used in generated code line: {code} and cannot be executed. If"
            f" you require a builtin package, reach out to the Vizro team."
        )

@lingyielia lingyielia merged commit f1e68ae into main Sep 20, 2024
39 of 40 checks passed
@lingyielia lingyielia deleted the poc/vizro_ai_charts_refactor branch September 20, 2024 13:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Vizro-AI 🤖 Issue/PR that addresses Vizro-AI package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants