Skip to content

Added return_df parameter to run_model_bpa_bulk #580

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

AkhilGrandhi
Copy link

Added a return_df parameter of boolean type. If set to True, the function will return a DataFrame containing bpa_rules.

Added a return_df parameter of boolean type. If set to True, the function will return a DataFrame containing bpa_rules.
@AkhilGrandhi
Copy link
Author

@microsoft-github-policy-service agree

@AkhilGrandhi AkhilGrandhi changed the title Added return_df parameter to return the result as a df Added return_df parameter to run_model_bpa_bulk Mar 20, 2025
@a-holm
Copy link

a-holm commented Mar 30, 2025

Thanks for adding the return_df feature! Being able to get the results back as a DataFrame programmatically is a useful addition.

However, I've identified a couple of issues with the current implementation:

  1. Premature Return: The return df statement is currently placed inside the main loop over workspaces (src/sempy_labs/_model_bpa_bulk.py, around line 190 in the diff). This means if return_df=True, the function will exit after processing and saving results for only the first workspace, instead of collecting results from all specified workspaces (if I understand the code correctly). To return the complete results, the aggregation logic needs adjustment (e.g., accumulate all results into a master DataFrame) and the return statement should be moved after the workspace loop finishes.
  2. Missing Documentation: The new return_df parameter needs to be added to the function's docstring to explain its purpose and usage.

Could you please adjust the logic to ensure all workspace results are collected before returning when return_df=True and update the docstring accordingly? It would probably make it easier for the microsoft fellas.

Could you provide the updated code so I can review it and refine the comments if needed?
@AkhilGrandhi
Copy link
Author

Hi @a-holm

Thanks for the review! I have now updated the code with the correct logic and proper comments. Please review and apply the changes.

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