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.

Sign up for GitHub

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

Wait for GHA eval results instead of falling back to local evaluation #451

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Wait for GHA eval results instead of falling back to local evaluation #451

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
32db2a2
Wait for GHA eval results instead of falling back to local evaluation
GaetanLepage Jan 5, 2025
c83b36e
Remove warning from github eval fetching
GaetanLepage Jan 5, 2025
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
17 changes: 1 addition & 16 deletions nixpkgs_review/github.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
from pathlib import Path
from typing import IO, Any, override

from .utils import System, warn
from .utils import System


def pr_url(pr: int) -> str:
Expand Down Expand Up @@ -175,32 +175,17 @@ def get_github_action_eval_result(
workflow_run["artifacts_url"],
)["artifacts"]

found_comparison = False
for artifact in artifacts:
if artifact["name"] != "comparison":
continue
found_comparison = True
changed_paths: Any = self.get_json_from_artifact(
workflow_id=artifact["id"],
json_filename="changed-paths.json",
)
if changed_paths is None:
warn(
f"Found comparison artifact, but no changed-paths.json in workflow {workflow_run['html_url']}"
)
continue
if (path := changed_paths.get("rebuildsByPlatform")) is not None:
assert isinstance(path, dict)
return path

if not found_comparison:
if workflow_run["status"] == "queued":
warn(
f"Found eval workflow run, but evaluation is still work in progress: {workflow_run['html_url']}"
)
else:
warn(
f"Found eval workflow run, but no comparison artifact in {workflow_run['html_url']}."
)

return None
25 changes: 18 additions & 7 deletions nixpkgs_review/review.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
import subprocess
import sys
import tempfile
import time
from dataclasses import dataclass, field
from enum import Enum
from pathlib import Path
Expand Down Expand Up @@ -296,14 +297,24 @@ def build_pr(self, pr_number: int) -> dict[System, list[Attr]]:
pr = self.github_client.pull_request(pr_number)

packages_per_system: dict[System, set[str]] | None = None
if self.use_github_eval and all(system in PLATFORMS for system in self.systems):
# Attempt to fetch the GitHub actions evaluation result
print("-> Attempting to fetch eval results from GitHub actions")
packages_per_system = self.github_client.get_github_action_eval_result(pr)

if packages_per_system is not None:
print("-> Successfully fetched rebuilds: no local evaluation needed")
if self.use_github_eval:
assert all(system in PLATFORMS for system in self.systems)
print("-> Fetching eval results from GitHub actions")

packages_per_system = self.github_client.get_github_action_eval_result(pr)
Copy link
Owner

Choose a reason for hiding this comment

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

I believe this also returns None if the evaluation failed. However in this case we shouldn't wait as we would wait forever.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should we crash in this case then ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This could be another reason why we would want to wait 'within' the github function. I.e. we would wait only in the case where we know that the results are currently computed.

if packages_per_system is None:
timeout: int = 10
print(f"...Results are not (yet) available. Retrying in {timeout}s")
Copy link
Owner

@Mic92 Mic92 Jan 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Maybe just print this message just once and just add a dot add the end each time it's not finished to indicate progress.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea ! Done.

while packages_per_system is None:
print(".", end="")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This doesn't work. Nothing is printed. If I remove end = "", it works.

Copy link
Owner

Choose a reason for hiding this comment

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

I think it's line buffered. Try flushing sys.stdout.flush()

sys.stdout.flush()
time.sleep(timeout)
packages_per_system = (
self.github_client.get_github_action_eval_result(pr)
)
print()

print("-> Successfully fetched rebuilds: no local evaluation needed")
else:
packages_per_system = None

Expand Down
Loading