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

Fix operation group status #774

Merged
merged 9 commits into from
Oct 19, 2023
Merged

Fix operation group status #774

merged 9 commits into from
Oct 19, 2023

Conversation

b-butler
Copy link
Member

@b-butler b-butler commented Oct 13, 2023

Description

Correct the selection of groups in status functions and splits _gather_flow_groups into 2 functions with clearer names.

Motivation and Context

Resolves: #773

Checklist:

@b-butler b-butler requested review from a team as code owners October 13, 2023 18:58
@b-butler b-butler requested review from bdice and Tobias-Dwyer and removed request for a team October 13, 2023 18:58
@bdice
Copy link
Member

bdice commented Oct 13, 2023

@b-butler It looks like this PR got smushed together with #739? Let me know if this is correct before I review.

@b-butler
Copy link
Member Author

@bdice yeah. I will fix with a rebase. 🙃

- _gather_selected_flow_groups: which selects groups regardless of
  executability
- _gather_executable_flow_groups: which defaults to singleton groups and
  only allows executable outputs or errors.
@b-butler
Copy link
Member Author

@bdice fixed. Sorry about that.

@codecov
Copy link

codecov bot commented Oct 13, 2023

Codecov Report

Merging #774 (63f030c) into main (74de383) will decrease coverage by 0.04%.
The diff coverage is 94.44%.

@@            Coverage Diff             @@
##             main     #774      +/-   ##
==========================================
- Coverage   69.15%   69.11%   -0.04%     
==========================================
  Files          44       44              
  Lines        4295     4297       +2     
  Branches     1047      950      -97     
==========================================
  Hits         2970     2970              
- Misses       1114     1115       +1     
- Partials      211      212       +1     
Files Coverage Δ
flow/project.py 82.63% <94.44%> (-0.11%) ⬇️

Copy link
Member

@bdice bdice left a comment

Choose a reason for hiding this comment

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

This seems like it should work to me. Code seems fine. @javierbg Can you verify if this PR behaves as you expected in #773?

flow/project.py Outdated Show resolved Hide resolved
flow/project.py Outdated Show resolved Hide resolved
Co-authored-by: Bradley Dice <bdice@bradleydice.com>
@javierbg
Copy link
Contributor

javierbg commented Oct 17, 2023

I can confirm this fixes the main issue described in #773.

However, just to make it clear, the related bug mentioned in #773. I think this is simple and related enough to just fix it here.

I would gladly do it, but I don't have permission to add commits to this PR... It would just be a simple change: just change this line to "status": operation_status.copy(),

For operations in multi-operation groups, the shared status dictionary
would get overwritten in status printing specifically the display name.
This results in only the last being printed.
@javierbg
Copy link
Contributor

javierbg commented Oct 18, 2023

As far as I can tell, this now completely fixes #773 . Thank you very much!

@bdice
Copy link
Member

bdice commented Oct 19, 2023

Awesome, thanks @javierbg and @b-butler. I'll let @b-butler merge this when ready.

@b-butler b-butler merged commit fd1f3d0 into main Oct 19, 2023
13 of 14 checks passed
@b-butler b-butler deleted the fix/operation-group-status branch October 19, 2023 15:57
@b-butler b-butler added this to the v0.27.0 milestone Jan 15, 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.

Restore grouped operations by default in status output
3 participants