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

Check if the manifest is already grouped #1041

Closed
2 tasks
zanete opened this issue Oct 7, 2024 · 8 comments · Fixed by #1108
Closed
2 tasks

Check if the manifest is already grouped #1041

zanete opened this issue Oct 7, 2024 · 8 comments · Fixed by #1108
Assignees
Milestone

Comments

@zanete
Copy link

zanete commented Oct 7, 2024

Why:
To resolve errors in if-check script and prevent error in edge case where user regroups an already-grouped manifest.

What:

Add a check to see if the manifest is already grouped according to the config. If the manifest is already grouped according to the given config, we should simply skip the regroup step.

The current behaviour, that we want to fix, is that the manifest regroup feature executes even the manifest is already grouped, which leads to additional, unnecessary levels of nesting.

Scope of work:

  • implement acceptance criteria
  • test cases added

Acceptance criteria

Scenario 1:

The following manifest:

# start
name: regroup
description: successful path
initialize:
  plugins: {}
tree:
  children:
    my-app:
      pipeline:
        regroup:
          - cloud/region
          - cloud/instance-type
      children:
        uk-west:
          children:
            A1:
              inputs:
                - timestamp: 2023-07-06T00:00
                  duration: 300
                  cloud/instance-type: A1
                  cloud/region: uk-west
                  cpu/utilization: 99
                - timestamp: 2023-07-06T05:00
                  duration: 300
                  cloud/instance-type: A1
                  cloud/region: uk-west
                  cpu/utilization: 23
                - timestamp: 2023-07-06T10:00
                  duration: 300
                  cloud/instance-type: A1
                  cloud/region: uk-west
                  cpu/utilization: 12
            B1:
              inputs:
                - timestamp: 2023-07-06T00:00
                  duration: 300
                  cloud/instance-type: B1
                  cloud/region: uk-west
                  cpu/utilization: 11
                - timestamp: 2023-07-06T05:00
                  duration: 300
                  cloud/instance-type: B1
                  cloud/region: uk-west
                  cpu/utilization: 67
                - timestamp: 2023-07-06T10:00
                  duration: 300
                  cloud/instance-type: B1
                  cloud/region: uk-west
                  cpu/utilization: 1

Should return the same exact file when run using if-run --regroup -m <manifest> (except for adding an execution node to context)

@zanete zanete added this to IF Oct 7, 2024
@zanete zanete converted this from a draft issue Oct 7, 2024
@zanete zanete moved this from Ready to In Refinement in IF Oct 7, 2024
@zanete zanete added this to the IF 1.0 milestone Oct 7, 2024
@jmcook1186
Copy link
Contributor

@narekhovhannisyan added some detail to the ticket

@zanete zanete moved this from In Refinement to Ready in IF Oct 8, 2024
@zanete zanete moved this from Ready to In Progress in IF Oct 8, 2024
@zanete
Copy link
Author

zanete commented Oct 10, 2024

Expecting a PR today

@zanete zanete moved this from In Progress to Parked in IF Oct 15, 2024
@zanete
Copy link
Author

zanete commented Oct 15, 2024

This issue needs to be re-refined, especially after a possible resolution will be found in relation to time grid

@zanete zanete moved this from Parked to Ready in IF Nov 4, 2024
@zanete zanete moved this from Ready to In Progress in IF Nov 4, 2024
@zanete
Copy link
Author

zanete commented Nov 4, 2024

@narekhovhannisyan please describe why this needed to be re-refined, where was the block why PR couldn't be created, if you can support by adding the slack conversation 🙏

@zanete
Copy link
Author

zanete commented Nov 8, 2024

@narekhovhannisyan please check this one today so you have stuff to do next week 🙏

@narekhovhannisyan
Copy link
Member

@zanete @jmcook1186 Since regroup logic is in the compute phase, the plugin is executed on the leaf node, so we can't check if it has nested children (for skipping purposes).

@zanete
Copy link
Author

zanete commented Nov 18, 2024

assuming that the solution is

    1. Check if regroup is enabled
    1. check the regroup criteria
    1. check if the subtree already is in the regrouped structure
    1. if yes, then skip traversing the subtree
    1. do this for all subtrees.

Estimate is about a couple of days

@zanete zanete moved this from In Refinement to Ready in IF Dec 2, 2024
@zanete zanete moved this from Ready to In Progress in IF Dec 11, 2024
@zanete
Copy link
Author

zanete commented Dec 17, 2024

Status update: Expecting a PR today 💪

@narekhovhannisyan narekhovhannisyan moved this from In Progress to Pending Review in IF Dec 17, 2024
@narekhovhannisyan narekhovhannisyan linked a pull request Dec 17, 2024 that will close this issue
6 tasks
@github-project-automation github-project-automation bot moved this from Pending Review to Done in IF Dec 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

3 participants