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

Implement extract_cellset_async #1030

Merged
merged 4 commits into from
Jan 26, 2024
Merged

Conversation

MariusWirtz
Copy link
Collaborator

Implement extract_cellset_async and expose function through execute_mdx and execute_view leverages extract_cellset_axes_raw_async and extract_cellset_cells_raw_asyncfunctions

Still lacking test cases but performance improvements are already measurable.

@MariusWirtz
Copy link
Collaborator Author

@vmitsenko
Is this what you had in mind when you introduced extract_cellset_axes_raw_async and extract_cellset_cells_raw_async in August last year?

@MariusWirtz MariusWirtz requested a review from vmitsenko January 18, 2024 22:25
@vmitsenko
Copy link
Collaborator

@MariusWirtz
Yes, exactly. There have also been ideas about incorporating it directly in "extract_cellset_raw" with an additional parameter to avoid creating a lot of additional "..._async", but there are doubts whether this would be a coherent approach.

and expose function through `execute_mdx` and `execute_view`
leverages `extract_cellset_axes_raw_async` and `extract_cellset_cells_raw_async`functions
@MariusWirtz MariusWirtz force-pushed the feature/extract-cellset-async branch from ddac11e to 31558ca Compare January 23, 2024 06:52
@MariusWirtz
Copy link
Collaborator Author

I see. The alternative to be introducing another _async function would be to add max_workers to the execute_mdx function.

I will stick to currently established patterns. I think long term we can deprecate the _async functions

@MariusWirtz MariusWirtz marked this pull request as ready for review January 23, 2024 09:44
@MariusWirtz
Copy link
Collaborator Author

@MariusWirtz Yes, exactly. There have also been ideas about incorporating it directly in "extract_cellset_raw" with an additional parameter to avoid creating a lot of additional "..._async", but there are doubts whether this would be a coherent approach.

Actually, adding the max_workers and async_axis arguments to the execute_mdx and execute_view simplifies the development and maintenance of the test cases.
So, I already included it in the PR.

Copy link
Collaborator

@vmitsenko vmitsenko left a comment

Choose a reason for hiding this comment

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

Small changes added, all tests passed successfully.

@MariusWirtz MariusWirtz merged commit f7d99e8 into master Jan 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants