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

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

Merged
merged 11 commits into from
Mar 15, 2024

Conversation

kevinmessiaen
Copy link
Member

@kevinmessiaen kevinmessiaen commented Mar 7, 2024

Always send the project_key when uploading/downloading tests

Copy link

linear bot commented Mar 7, 2024

@kevinmessiaen kevinmessiaen marked this pull request as ready for review March 7, 2024 09:07
Copy link
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?

@@ -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
Member

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

@@ -481,16 +479,14 @@ def do_nothing(row):


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

Choose a reason for hiding this comment

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

idem.

@@ -149,17 +150,17 @@ def test_download_callable_function(cf: Artifact):
do_nothing, # Transformation
],
)
def test_download_global_callable_function_from_module(cf: Artifact):
def test_download_callable_function_from_module(cf: Artifact):
with MockedClient(mock_all=False) as (client, mr):
cf.meta.uuid = str(uuid.uuid4()) # Regenerate a UUID to ensure not loading from registry
cache_dir = get_local_cache_callable_artifact(artifact=cf)

requested_urls = []
# Prepare global URL
Copy link
Member

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
Copy link
Member Author

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

Copy link

sonarcloud bot commented Mar 11, 2024

Copy link
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
16 checks passed
@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
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants