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

perf: Improve performance of Chart.from_dict #3383

Merged
merged 3 commits into from
Mar 26, 2024

Conversation

RobinL
Copy link
Contributor

@RobinL RobinL commented Mar 24, 2024

Closes #3382

This PR substantially improves performance of alt.Chart.from_dict. See #3382 for a detailed description and reprex.

Grateful for any feedback on this PR, e.g. whether the team would prefer I leave the current function as is, or use functools.lru_cache, instead of the top-level jsonschema_version_str = importlib_version("jsonschema") statement. Or some other solution.

WIP whilst I make sure the builds/tests pass

@RobinL RobinL changed the title (WIP) Improve performance of Chart.from_dict Improve performance of Chart.from_dict Mar 24, 2024
@RobinL RobinL changed the title Improve performance of Chart.from_dict (WIP) Improve performance of Chart.from_dict Mar 24, 2024
@jonmmease
Copy link
Contributor

Thanks for the investigation and the fix @RobinL! I think this is a reasonable way to go, but would like to hear from @binste as well

@RobinL
Copy link
Contributor Author

RobinL commented Mar 24, 2024

Thanks very much for the quick response, will await feedback from binste.

[Edit - apologies for the force push, had accidentally committed the schemapi.py from utils rather than tools]

@RobinL RobinL changed the title (WIP) Improve performance of Chart.from_dict Improve performance of Chart.from_dict Mar 24, 2024
@joelostblom joelostblom changed the title Improve performance of Chart.from_dict perf: Improve performance of Chart.from_dict Mar 24, 2024
@jonmmease
Copy link
Contributor

This is straightforward enough that with the passing tests I'm fine merging it. We can make any adjustments before the 5.3.0 release if anyone want to weight in afterward.

@jonmmease jonmmease merged commit e20cdef into vega:main Mar 26, 2024
10 checks passed
@binste
Copy link
Contributor

binste commented Mar 28, 2024

Nice catch @RobinL! Thanks for finding and fixing it :)

Thanks Jon for the review! Fully agree, changes look good so we can release it in 5.3.0 as-is.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

alt.Chart.from_dict slow due to large number of calls to utils.schemaapi._use_referencing_library
4 participants