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

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

Merged
merged 2 commits into from
Oct 28, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 59 additions & 0 deletions docs/managers.rst
Original file line number Diff line number Diff line change
Expand Up @@ -85,3 +85,62 @@ Kebechet works as a part of Thoth Ecosystem, please raise an issue or add the
new manager to the `KebechetGithubAppInstallations
<https://github.com/thoth-station/storages/blob/15ed39ef6c8d7bf58037046f3bab2465c5c4bb22/thoth/storages/graph/models.py#L1434>`_
table.


Configure different managers per Overlays
-----------------------------------------

- Add allow and disallow to each manager's configuration options, where the user can specify which manager is (dis)allowed for each overlay.

Example: .thoth.yaml

.. code-block:: yaml

host: khemenu.thoth-station.ninja
tls_verify: true
requirements_format: pipenv
overlays_dir: overlays

runtime_environments:
- name: ps-nlp
operating_system:
name: ubi
version: "8"
python_version: "3.8"
recommendation_type: latest

- name: ps-nlp-pytorch
operating_system:
name: ubi
version: "8"
python_version: "3.8"
recommendation_type: latest

- name: ps-nlp-tensorflow
operating_system:
name: ubi
version: "8"
python_version: "3.8"
recommendation_type: latest

- name: ps-nlp-tensorflow-gpu
operating_system:
name: ubi
version: "8"
python_version: "3.8"
recommendation_type: latest
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
22 changes: 20 additions & 2 deletions kebechet/kebechet_runners.py
Original file line number Diff line number Diff line change
Expand Up @@ -172,11 +172,29 @@ def run(
return

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

ack

managers = config.managers

runtime_environments: List[str]
if runtime_environment:
runtime_environments = [runtime_environment]
elif config.config.get("overlays_dir"):
runtime_environments = [
e["name"] for e in config.config.get("runtime_environments")
]
else:
runtime_environments = [config.config["runtime_environments"][0]["name"]]
for manager in managers:
# We do pops on dict, which changes it. Let's create a soft duplicate so if a user uses
# YAML references, we do not break.
manager = dict(manager)
disallow_run_times = manager.get("env_disallow_list", [])
allowed_run_times = manager.get("env_allow_list", [])
if disallow_run_times:
runtime_environments = list(
set(runtime_environments) - set(disallow_run_times)
)
if allowed_run_times:
runtime_environments = list(
set(runtime_environments).intersection(set(allowed_run_times))
)
harshad16 marked this conversation as resolved.
Show resolved Hide resolved
try:
manager_name = manager.pop("name")
except Exception:
Expand Down Expand Up @@ -212,7 +230,7 @@ def run(
service_type=service_type,
parsed_payload=parsed_payload,
metadata=metadata,
runtime_environment=runtime_environment,
runtime_environments=runtime_environments,
)
instance.run(**manager_configuration)
except Exception as exc: # noqa F841
Expand Down
4 changes: 2 additions & 2 deletions kebechet/managers/manager.py
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ def __init__(
service_type: str,
parsed_payload: Optional[dict] = None,
metadata: Optional[dict] = None,
runtime_environment: Optional[str] = None,
runtime_environments: List[str] = None,
):
"""Initialize manager instance for talking to services."""
self.service_url: str = service.instance_url # type: ignore
Expand All @@ -66,7 +66,7 @@ def __init__(
self.project = service.get_project(namespace=self.owner, repo=self.repo_name)
self._repo: git.Repo = None
self.metadata = metadata
self.runtime_environment = runtime_environment
self.runtime_environments = runtime_environments

@property
def repo(self):
Expand Down
12 changes: 1 addition & 11 deletions kebechet/managers/thoth_advise/thoth_advise.py
Original file line number Diff line number Diff line change
Expand Up @@ -294,7 +294,6 @@ def run(self, labels: list, analysis_id=None):
self._issue_list = self.project.get_issue_list()
self._close_advise_issues4users_lacking_perms()
self._tracking_issue = self._close_all_but_oldest_issue()
runtime_environments: typing.List[typing.Tuple[str, Issue]]

if analysis_id is None:
if self._tracking_issue is None:
Expand All @@ -311,16 +310,7 @@ 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 = [
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

ack,done

e["name"] for e in thoth_config["runtime_environments"]
]
else:
runtime_environments = [
thoth_config["runtime_environments"][0]["name"]
]
harshad16 marked this conversation as resolved.
Show resolved Hide resolved
for e in runtime_environments:
for e in self.runtime_environments or []:
try:
analysis_id = lib.advise_here(
nowait=True,
Expand Down
26 changes: 8 additions & 18 deletions kebechet/managers/update/update.py
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@
import re
import json
import typing
from typing import Optional, List
from typing import Optional
from itertools import chain
from functools import partial
from datetime import datetime
Expand Down Expand Up @@ -111,6 +111,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"
Copy link
Member

Choose a reason for hiding this comment

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

Why is this line added here?

Copy link
Member Author

Choose a reason for hiding this comment

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

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

super().__init__(*args, **kwargs)

@property
Expand Down Expand Up @@ -738,26 +739,14 @@ def run(self, labels: list = []) -> Optional[dict]:
runtime_environment_names = [
e["name"] for e in thoth_config.list_runtime_environments()
]

overlays_dir = thoth_config.content.get("overlays_dir")

runtime_environments: List[Optional[str]]
if self.runtime_environment:
if self.runtime_environment not in runtime_environment_names:
results: dict = {}
for e in self.runtime_environments or []:
if e 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]
else:
if overlays_dir:
runtime_environments = runtime_environment_names
elif runtime_environment_names:
runtime_environments = [runtime_environment_names[0]]
else:
runtime_environments = [None]
Copy link
Member

Choose a reason for hiding this comment

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

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


results: dict = {}
for e in runtime_environments:
continue
self.runtime_environment = e or "default"
close_no_management_issue = partial(
self.close_issue_and_comment,
Expand Down Expand Up @@ -793,7 +782,8 @@ def run(self, labels: list = []) -> Optional[dict]:
except Exception:
_LOGGER.exception(
f"Failed to delete branch "
f"{_UPDATE_BRANCH_NAME.format(env_name=self.runtime_environment)}, trying to continue"
f"{_UPDATE_BRANCH_NAME.format(env_name=self.runtime_environment)}, "
f"trying to continue"
)
elif to_rebase:
for pr in to_rebase:
Expand Down