-
Notifications
You must be signed in to change notification settings - Fork 244
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
[batch] re-allow sending kwargs to python jobs #13505
Conversation
CHANGELOG: Fix bug introduced in 0.2.117 by commit `c9de81108` which prevented the passing of keyword arguments to Python jobs. This manifested as "ValueError: too many values to unpack".
hail/python/hailtop/batch/job.py
Outdated
@@ -877,6 +877,19 @@ async def _compile(self, local_tmpdir, remote_tmpdir, *, dry_run=False): | |||
return True | |||
|
|||
|
|||
UnpreparedArg = '_resource.ResourceType' | List['UnpreparedArg'] | Tuple['UnpreparedArg', ...] | Dict[str, 'UnpreparedArg'] | Any |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we have to wait until 3.10 for this unfortunately.
1cb6323
to
f5755b8
Compare
@@ -448,3 +448,6 @@ def __str__(self): | |||
|
|||
def __repr__(self): | |||
return self._uid # pylint: disable=no-member | |||
|
|||
|
|||
ResourceType = PythonResult | ResourceFile | ResourceGroup |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still have pipes here. I didn't mean to get rid of the types altogether though, just replace the pipes with a Union
Ah, my bad, I thought you mean the recursive types with strings. I didn't realize the pipes were new. That's what I get for not clicking links. |
This reverts commit 19807da.
merge main, actually address comments, ensure pyright is satisfied
@@ -12,15 +12,17 @@ def to_dict(self): | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't entirely understand why pyright runs on everything but it does and it dislikes the formatting of this file.
@@ -5,7 +5,7 @@ | |||
|
|||
from hailtop.aiocloud.aiogoogle import get_gcs_requester_pays_configuration | |||
from hailtop.aiocloud.aiogoogle.user_config import get_spark_conf_gcs_requester_pays_configuration, spark_conf_path | |||
from hailtop.config.user_config import ConfigVariable, configuration_of | |||
from hailtop.config import ConfigVariable, configuration_of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pyright also disliked this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and it's right: ConfigVariable is not part of user_config.py
CHANGELOG: Fix bug introduced in 0.2.117 by commit
c9de81108
which prevented the passing of keyword arguments to Python jobs. This manifested as "ValueError: too many values to unpack".We also weren't preserving tuples. They become lists. I fixed that too. I also added some types and avoided an is instance by encoding the necessary knowledge.