Skip to content

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