Fix direct usage of TransformerModel #619
Conversation
| @@ -112,6 +112,8 @@ def test(session, pandas_version: str): | |||
| "python", | |||
| "-m", | |||
| "pytest", | |||
| "-ra", | |||
There was a problem hiding this comment.
This reports all test except passes, which is useful to see why a specific would have been skipped.
| @@ -112,6 +112,8 @@ def test(session, pandas_version: str): | |||
| "python", | |||
| "-m", | |||
| "pytest", | |||
| "-ra", | |||
| "--tb=native", | |||
There was a problem hiding this comment.
This shows the Python native traceback, instead of the overly verbose pytest tracebacks that display the source code (I don't need it, I can see it in my editor).
| transformers[torch]>=4.12.0,<=4.27.4; python_version<'3.11' | ||
| transformers[torch]>=4.31.0,<=4.33.2; python_version<'3.11' |
There was a problem hiding this comment.
A newer version of transformers is needed to get access token support. This was already upgraded in setup.py, but not here. I'll have to avoid the duplication in the future.
| @@ -103,31 +112,17 @@ def download_model_and_start_deployment(tmp_dir, quantize, model_id, task): | |||
|
|
|||
|
|
|||
| class TestPytorchModel: | |||
| def __init__(self): | |||
There was a problem hiding this comment.
pytest will not run any test classes when an __init__ method is defined. So we had a test but it was not run, and pytest only warns about it. I've switched to a pytest fixture instead.
eland/ml/pytorch/transformers.py
Outdated
| @@ -584,7 +584,7 @@ def __init__( | |||
| self, | |||
| *, | |||
| model_id: str, | |||
| access_token: Optional[str], | |||
| access_token: Optional[str] = None, | |||
There was a problem hiding this comment.
The order of access_token does not matter because those are kwargs-only parameter that can't be passed conditionally.
There was a problem hiding this comment.
Stylistically, I always prefer default args are at the end, but I don't know if this is more or less Pythonic. Either way is fine, just curious
There was a problem hiding this comment.
Ah, that's an interesting consideration. I don't know if there's one choice that's more Pythonic than the other (I haven't seen enough such cases to tell).
I don't have a strong preference so I am happy to change this.
eland/ml/pytorch/transformers.py
Outdated
| @@ -584,7 +584,7 @@ def __init__( | |||
| self, | |||
| *, | |||
| model_id: str, | |||
| access_token: Optional[str], | |||
| access_token: Optional[str] = None, | |||
There was a problem hiding this comment.
Stylistically, I always prefer default args are at the end, but I don't know if this is more or less Pythonic. Either way is fine, just curious
|
Thanks. My intention is always to make a single change per pull request. In that case, there is indeed a single changed line in the core Eland package. The rest of the pull request fixes the tests that should have caught the bug but were skipped due to a pytest technicality. Does that make sense? |
|
Tested the latest commit with |
Closes #617