Add 'allow' and 'disallow' setting to each manager #1156

shreekarSS · 2022-10-11T16:01:55Z

Signed-off-by: Shreekara sshreeka@redhat.com

Related Issues and Dependencies

Fixes: #1126
…

This Pull Request implements

New config setting for managers in .thoth.yaml
For ex:

managers:

name: thoth-advise
//Do not run this manager in the ps-nlp-tensorflow-gpu overlay
env_disallow_list:
- ps-nlp-tensorflow-gpu
  configuration:
  labels: [bot]
name: update
//Run this manager in the ps-nlp-tensorflow-gpu overlay (only)
env_allow_list:
- ps-nlp-tensorflow-gpu
  configuration:
  labels: [bot]
name: info
//No allow/disallow present: this manager applies to all runtimes
...

KPostOffice

Good start!

I think it might make sense to make managerbase take runtime_environments as a list and have it calculated this way.

See here

KPostOffice · 2022-10-12T13:38:46Z

kebechet/managers/thoth_advise/thoth_advise.py

+                    allowed_run_times = managers[0].get("allow", None)
+                    if disallow_run_times:
+                        runtime_environments = list(
+                            set(runtime_environments) ^ set(disallow_run_times)


I think this should just be set subtraction -

KPostOffice · 2022-10-12T13:39:21Z

kebechet/managers/thoth_advise/thoth_advise.py

+                            set(runtime_environments) ^ set(disallow_run_times)
+                        )
+                    if allowed_run_times:
+                        runtime_environments = allowed_run_times


I think this would be better as set intersection &

KPostOffice · 2022-10-12T13:40:54Z

kebechet/managers/thoth_advise/thoth_advise.py

@@ -316,6 +316,15 @@ def run(self, labels: list, analysis_id=None):
                    runtime_environments = [
                        e["name"] for e in thoth_config["runtime_environments"]
                    ]
+                    managers = thoth_config.get("managers")
+                    disallow_run_times = managers[0].get("disallow", None)


This should be per manager, not global to all managers

ack, thanks

KPostOffice

kebechet/kebechet/kebechet_runners.py

Line 208 in 356af8e

if manager_configuration.pop("enabled", True):

Let's try getting a list of runtime environments here and passing it directly to the manager. Replacing the current parameter runtime_environment with runtime_environments that takes a list. This should help with code duplication. It will require a bit more reworking though.

KPostOffice · 2022-10-12T18:17:04Z

kebechet/managers/thoth_advise/thoth_advise.py

+                    for mgr in managers:
+                        if mgr.get("name") == "thoth-advise":
+                            disallow_run_times = mgr.get("disallow", None)
+                            allowed_run_times = mgr.get("allow", None)


This shouldn't have to be done here, should be passed through the managers configuration variables.

Signed-off-by: Shreekara <sshreeka@redhat.com>

KPostOffice · 2022-10-14T17:57:03Z

kebechet/kebechet_runners.py

@@ -172,11 +172,26 @@ def run(
        return

    managers = config.managers
-
+    if config.config.get("overlays_dir"):
+        runtime_environment = [


Let's change the name to runtime_environments

KPostOffice

Looks good. A few comments

KPostOffice · 2022-10-14T17:58:38Z

kebechet/managers/update/update.py

-                if self.runtime_environment not in runtime_environment_names:
-                    # This is not a warning as it is expected when users remove and change runtime_environments
-                    _LOGGER.info("Requested runtime does not exist in target repo.")
-                    return None
-                runtime_environments = [self.runtime_environment]


We should move this check down into the for loop and just replace return None with continue

KPostOffice · 2022-10-14T18:02:50Z

kebechet/kebechet_runners.py

+        disallow_run_times = manager.get("disallow", None)
+        allowed_run_times = manager.get("allow", None)


These are technically mutually exclusive. Not sure how important it is to users though.

KPostOffice · 2022-10-14T18:05:30Z

kebechet/kebechet_runners.py

@@ -172,11 +172,26 @@ def run(
        return



We need to respect the passed runtime_environment. We could also allow the user to pass a list of runtime_environments in the future, I don't think we need to do this now though.

Signed-off-by: Shreekara <sshreeka@redhat.com>

KPostOffice

One comment in two spots, otherwise it looks good!

KPostOffice · 2022-10-17T17:04:34Z

kebechet/managers/thoth_advise/thoth_advise.py

-
-                                If this project does not use requirements.txt or Pipfile then remove thoth-advise
-                                manager from your .thoth.yaml configuration."""
+                if self.runtime_environments:


This check shouldn't be necessary if the default would be an empty list. If the default is None however, you could change the for loop to:

for e in self.runtime_environments or []:

Here and in update manager

I still see the if statement in the current PR. I think it can be handled in the for as @KPostOffice mention.

harshad16

Please update documentation for this feature in this PR itself.

shreekarSS · 2022-10-17T21:07:09Z

Please update documentation for this feature in this PR itself.

Ack, thanks ,fixed

harshad16

thanks for the great work.
dropped a few comments.

docs/managers.rst

harshad16 · 2022-10-18T03:39:03Z

kebechet/kebechet_runners.py

+        disallow_run_times = manager.get("disallow", None)
+        allowed_run_times = manager.get("allow", None)


for default: empty list [] might be better than None.

kebechet/kebechet_runners.py

kebechet/managers/thoth_advise/thoth_advise.py

harshad16

Thanks
/lgtm

PTAL @KPostOffice when you get some time.

KPostOffice

A few more comments

KPostOffice · 2022-10-20T19:52:36Z

kebechet/kebechet_runners.py

+    runtime_environments: List[str]
+    if runtime_environment:
+        runtime_environments = [runtime_environment]
+    if config.config.get("overlays_dir"):


KPostOffice · 2022-10-20T19:53:47Z

docs/managers.rst

+        managers:
+          - name: thoth-advise
+            # Do not run this manager in the ps-nlp-tensorflow-gpu overlay
+            disallow:


I'm thinking we should make the name more clear. What do you think about env_allow_list and env_disallow_list?

ack, will work on that

KPostOffice · 2022-10-20T19:57:02Z

kebechet/managers/update/update.py

@@ -109,6 +109,7 @@ def __init__(self, *args, **kwargs):
        # We do API calls once for merge requests and we cache them for later use.
        self._cached_merge_requests = None
        self._pr_list = []
+        self.runtime_environment = "default"


Why is this line added here?

previously , self.runtime_environment was a string, being used in https://github.com/thoth-station/kebechet/blob/master/kebechet/managers/update/update.py#L761 and many more places.
Since we are converting it list of strings, do not want to break the code since it's already being used in various methods, created a variable to keep it running

KPostOffice · 2022-10-20T19:58:20Z

kebechet/managers/update/update.py

-                elif runtime_environment_names:
-                    runtime_environments = [runtime_environment_names[0]]
-                else:
-                    runtime_environments = [None]


I think we lost the "default" functionality here. I think the for loop should probably be default [None] instead of []

KPostOffice · 2022-10-26T19:16:39Z

/lgtm

codificat

Found one minor thing remaining

codificat · 2022-10-27T18:15:59Z

kebechet/managers/thoth_advise/thoth_advise.py

-
-                                If this project does not use requirements.txt or Pipfile then remove thoth-advise
-                                manager from your .thoth.yaml configuration."""
+                if self.runtime_environments:


I still see the if statement in the current PR. I think it can be handled in the for as @KPostOffice mention.

codificat · 2022-10-27T18:20:10Z

kebechet/managers/thoth_advise/thoth_advise.py

@@ -311,51 +311,43 @@ def run(self, labels: list, analysis_id=None):

                with open(".thoth.yaml", "r") as f:
                    thoth_config = yaml.safe_load(f)
-
-                if thoth_config.get("overlays_dir"):
-                    runtime_environments = [


As we are removing the local runtime_environments var, let's also remove its declaration above (line 297)

Signed-off-by: Shreekara <sshreeka@redhat.com>

KPostOffice · 2022-10-27T20:02:28Z

/approve

harshad16

/lgtm

lets get this in 👍

sesheta · 2022-10-28T15:49:36Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: harshad16, KPostOffice

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [KPostOffice,harshad16]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

sesheta added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 11, 2022

sesheta requested review from harshad16 and KPostOffice October 11, 2022 16:02

sesheta added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Oct 11, 2022

shreekarSS changed the title ~~[WIP] Add 'allow' and 'disallow' setting to each manager~~ Add 'allow' and 'disallow' setting to each manager Oct 11, 2022

sesheta removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 11, 2022

KPostOffice reviewed Oct 12, 2022

View reviewed changes

shreekarSS force-pushed the diffrent_mgrs_overlay branch from 8b9d789 to 77d466e Compare October 12, 2022 16:30

sesheta added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Oct 12, 2022

shreekarSS force-pushed the diffrent_mgrs_overlay branch from 77d466e to 5dd79b2 Compare October 12, 2022 16:31

sesheta added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 12, 2022

shreekarSS force-pushed the diffrent_mgrs_overlay branch from 5dd79b2 to 651f148 Compare October 12, 2022 18:12

KPostOffice reviewed Oct 12, 2022

View reviewed changes

shreekarSS added a commit to shreekarSS/kebechet that referenced this pull request Oct 14, 2022

Address thoth-station#1156 (review)

7f1dc53

Signed-off-by: Shreekara <sshreeka@redhat.com>

sesheta added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Oct 14, 2022

KPostOffice reviewed Oct 14, 2022

View reviewed changes

shreekarSS added a commit to shreekarSS/kebechet that referenced this pull request Oct 17, 2022

Address thoth-station#1156 (review)

69f11a0

Signed-off-by: Shreekara <sshreeka@redhat.com>

shreekarSS force-pushed the diffrent_mgrs_overlay branch 2 times, most recently from 69f11a0 to e0fb342 Compare October 17, 2022 16:42

KPostOffice reviewed Oct 17, 2022

View reviewed changes

shreekarSS force-pushed the diffrent_mgrs_overlay branch from e0fb342 to eed0a55 Compare October 17, 2022 17:26

harshad16 requested changes Oct 17, 2022

View reviewed changes

shreekarSS force-pushed the diffrent_mgrs_overlay branch from 0bb87a8 to c54451a Compare October 17, 2022 21:15

harshad16 reviewed Oct 18, 2022

View reviewed changes

shreekarSS force-pushed the diffrent_mgrs_overlay branch from c54451a to c9de68e Compare October 18, 2022 15:39

harshad16 reviewed Oct 18, 2022

View reviewed changes

sesheta assigned harshad16 Oct 18, 2022

sesheta added the lgtm Indicates that a PR is ready to be merged. label Oct 18, 2022

shreekarSS force-pushed the diffrent_mgrs_overlay branch from c9de68e to a099ae5 Compare October 18, 2022 15:43

sesheta removed the lgtm Indicates that a PR is ready to be merged. label Oct 18, 2022

shreekarSS force-pushed the diffrent_mgrs_overlay branch from a099ae5 to 698eb65 Compare October 19, 2022 21:08

KPostOffice reviewed Oct 20, 2022

View reviewed changes

shreekarSS force-pushed the diffrent_mgrs_overlay branch 2 times, most recently from 2339c93 to b771508 Compare October 25, 2022 15:52

sesheta assigned KPostOffice Oct 26, 2022

sesheta added the lgtm Indicates that a PR is ready to be merged. label Oct 26, 2022

codificat requested changes Oct 27, 2022

View reviewed changes

shreekarSS added 2 commits October 27, 2022 15:30

Add 'env_allow_list' and 'env_disallow_list' setting to each manager

e087552

Signed-off-by: Shreekara <sshreeka@redhat.com>

New feature update : Add 'allow' and 'disallow' setting to each manager

16c8374

Signed-off-by: Shreekara <sshreeka@redhat.com>

shreekarSS force-pushed the diffrent_mgrs_overlay branch from b771508 to 16c8374 Compare October 27, 2022 19:31

sesheta removed the lgtm Indicates that a PR is ready to be merged. label Oct 27, 2022

sesheta added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 27, 2022

harshad16 approved these changes Oct 28, 2022

View reviewed changes

sesheta added the lgtm Indicates that a PR is ready to be merged. label Oct 28, 2022

sesheta merged commit d9f3f6d into thoth-station:master Oct 28, 2022

harshad16 mentioned this pull request Nov 14, 2022

sprint production release v2022.11.14 thoth-station/thoth-application#2687

Closed

		disallow_run_times = manager.get("disallow", None)
		allowed_run_times = manager.get("allow", None)

Add 'allow' and 'disallow' setting to each manager #1156

Add 'allow' and 'disallow' setting to each manager #1156

Conversation

shreekarSS commented Oct 11, 2022 • edited Loading

Related Issues and Dependencies

This Pull Request implements

KPostOffice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KPostOffice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KPostOffice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KPostOffice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

harshad16 left a comment

Choose a reason for hiding this comment

shreekarSS commented Oct 17, 2022

harshad16 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

harshad16 left a comment

Choose a reason for hiding this comment

KPostOffice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KPostOffice commented Oct 26, 2022

codificat left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KPostOffice commented Oct 27, 2022

harshad16 left a comment

Choose a reason for hiding this comment

sesheta commented Oct 28, 2022

shreekarSS commented Oct 11, 2022 •

edited

Loading