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

Cleanup #176

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Cleanup #176

wants to merge 8 commits into from

Conversation

leseb
Copy link
Collaborator

@leseb leseb commented Nov 14, 2024

a429b67 fix: remove non-existent member
32e6ded fix: add missing import
17124a9 fix: add encoding to open call
6b15973 fix: remove unused imports
2503d86 fix: silence unused variables
abce864 fix: add timeout to requests.get
f416e44 fix: remove unused "device" variable
74012d3 fix: gen new pipeline and standalone

commit a429b67
Author: Sébastien Han seb@redhat.com
Date: Thu Nov 14 13:51:23 2024 +0100

fix: remove non-existent member

`subprocess.NoSuchProcess` does not exist. The `ProcessLookupError` case
is handled already by the subprocess package.

Also, no need to catch the general Exception.

Signed-off-by: Sébastien Han <seb@redhat.com>

commit 32e6ded
Author: Sébastien Han seb@redhat.com
Date: Thu Nov 14 14:18:23 2024 +0100

fix: add missing import

Signed-off-by: Sébastien Han <seb@redhat.com>

commit 17124a9
Author: Sébastien Han seb@redhat.com
Date: Thu Nov 14 14:19:07 2024 +0100

fix: add encoding to open call

Signed-off-by: Sébastien Han <seb@redhat.com>

commit 6b15973
Author: Sébastien Han seb@redhat.com
Date: Thu Nov 14 14:20:17 2024 +0100

fix: remove unused imports

Signed-off-by: Sébastien Han <seb@redhat.com>

commit 2503d86
Author: Sébastien Han seb@redhat.com
Date: Thu Nov 14 14:20:57 2024 +0100

fix: silence unused variables

Signed-off-by: Sébastien Han <seb@redhat.com>

commit abce864
Author: Sébastien Han seb@redhat.com
Date: Thu Nov 14 14:24:06 2024 +0100

fix: add timeout to requests.get

Wait 10sec for a response.

Signed-off-by: Sébastien Han <seb@redhat.com>

commit f416e44
Author: Sébastien Han seb@redhat.com
Date: Thu Nov 14 14:22:28 2024 +0100

fix: remove unused "device" variable

Signed-off-by: Sébastien Han <seb@redhat.com>

commit 74012d3
Author: Sébastien Han seb@redhat.com
Date: Thu Nov 14 14:29:05 2024 +0100

fix: gen new pipeline and standalone

Signed-off-by: Sébastien Han <seb@redhat.com>

@leseb leseb force-pushed the cleanup branch 2 times, most recently from 164b2f4 to adfcc06 Compare November 14, 2024 13:40
`subprocess.NoSuchProcess` does not exist. The `ProcessLookupError` case
is handled already by the subprocess package.

Also, no need to catch the general Exception.

Signed-off-by: Sébastien Han <seb@redhat.com>
Signed-off-by: Sébastien Han <seb@redhat.com>
Signed-off-by: Sébastien Han <seb@redhat.com>
Signed-off-by: Sébastien Han <seb@redhat.com>
Signed-off-by: Sébastien Han <seb@redhat.com>
Wait 10sec for a response.

Signed-off-by: Sébastien Han <seb@redhat.com>
Signed-off-by: Sébastien Han <seb@redhat.com>
Signed-off-by: Sébastien Han <seb@redhat.com>
Comment on lines +102 to 103
# from eval.mmlu import load_mmlu_results_op, run_mmlu_op
## from eval.mmlu import run_mmlu_op, load_mmlu_results_op
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not just remove these 2 lines? Why comment them out?

@@ -119,12 +118,11 @@ def pipeline(
base_model: str = BASE_MODEL,
# minimal subset of MMLU_TASKS
# mmlu_tasks_list: str = MMLU_TASKS_LIST,
model_dtype: str = MODEL_DTYPE,
# model_dtype: str = MODEL_DTYPE,
Copy link
Collaborator

Choose a reason for hiding this comment

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

just remove

@@ -9,7 +9,6 @@
CreatePVC,
DeletePVC,
mount_pvc,
set_image_pull_policy,
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is actually used here:

ilab-on-ocp/pipeline.py

Lines 420 to 421 in ac474e1

# uncomment if updating image with same tag
# set_image_pull_policy(run_mt_bench_task, "Always")

So I would not remove it. But probably we want to implement a better solution than having the user uncomment code.

@MichaelClifford
Copy link
Collaborator

Just a couple minor nits. Also, would like @sallyom to take a look as there are a few changes to the eval components.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants