Skip to content

GSK-2887 Upload tests with project when uploading a suite#1837

Merged
kevinmessiaen merged 11 commits intomulti-ml-workerfrom
GSK-2887
Mar 15, 2024
Merged

GSK-2887 Upload tests with project when uploading a suite#1837
kevinmessiaen merged 11 commits intomulti-ml-workerfrom
GSK-2887

Conversation

@kevinmessiaen
Copy link
Copy Markdown
Member

@kevinmessiaen kevinmessiaen commented Mar 7, 2024 •
edited
Loading

Always send the project_key when uploading/downloading tests

@linear
Copy link
Copy Markdown

linear Bot commented Mar 7, 2024

GSK-2887 Scope catalog to project

@kevinmessiaen kevinmessiaen marked this pull request as ready for review March 7, 2024 09:07
Inokinoki
Inokinoki requested changes Mar 7, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@Inokinoki Inokinoki left a comment

Choose a reason for hiding this comment

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

LGTM
Just some comments to be removed or polished.

Not related to this PR:
We will neither have global resources anymore, nor "internal worker". So we can also cleanup the Optional[GiskardClient], can we create a card for this?

Comment thread tests/communications/test_websocket_actor.py Outdated
@@ -430,12 +430,10 @@ def head_slice(df: pd.DataFrame) -> pd.DataFrame:


# FIXME: Internal worker cannot yet load callable due to yaml deserialization issue
Copy link
Copy Markdown
Member

@Inokinoki Inokinoki Mar 7, 2024

Choose a reason for hiding this comment

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

We can remove this line

Comment thread tests/communications/test_websocket_actor.py Outdated
@@ -481,16 +479,14 @@ def do_nothing(row):


# FIXME: Internal worker cannot yet load callable due to yaml deserialization issue
Copy link
Copy Markdown
Member

@Inokinoki Inokinoki Mar 7, 2024

Choose a reason for hiding this comment

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

idem.

Comment thread tests/test_artifact_download.py Outdated
cache_dir = get_local_cache_callable_artifact(artifact=cf)

requested_urls = []
# Prepare global URL
Copy link
Copy Markdown
Member

@Inokinoki Inokinoki Mar 7, 2024

Choose a reason for hiding this comment

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

Can be changed to prepare URLs, cuz we have no more global artefacts

@kevinmessiaen kevinmessiaen requested a review from Inokinoki March 11, 2024 06:40
@kevinmessiaen
Copy link
Copy Markdown
Member Author

kevinmessiaen commented Mar 11, 2024

LGTM Just some comments to be removed or polished.

Not related to this PR: We will neither have global resources anymore, nor "internal worker". So we can also cleanup the Optional[GiskardClient], can we create a card for this?

I've updated th PR and remove the optional client

@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud Bot commented Mar 11, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

Inokinoki
Inokinoki approved these changes Mar 14, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@Inokinoki Inokinoki left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks!

@kevinmessiaen kevinmessiaen merged commit 5cccfc5 into multi-ml-worker Mar 15, 2024
@kevinmessiaen kevinmessiaen deleted the GSK-2887 branch March 15, 2024 02:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Inokinoki Inokinoki Inokinoki approved these changes

Assignees

@kevinmessiaen kevinmessiaen

Labels

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@kevinmessiaen @Inokinoki @Hartorn