-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
feat: Python SDK publish #7363
feat: Python SDK publish #7363
Conversation
Signed-off-by: flaviuvadan <[email protected]>
…files are listed in gitignore anyway Signed-off-by: flaviuvadan <[email protected]>
Signed-off-by: flaviuvadan <[email protected]>
Signed-off-by: flaviuvadan <[email protected]>
Signed-off-by: flaviuvadan <[email protected]>
Signed-off-by: flaviuvadan <[email protected]>
…f the generated Python SDK Signed-off-by: flaviuvadan <[email protected]>
Signed-off-by: flaviuvadan <[email protected]>
Signed-off-by: flaviuvadan <[email protected]>
Signed-off-by: flaviuvadan <[email protected]>
Signed-off-by: flaviuvadan <[email protected]>
Signed-off-by: flaviuvadan <[email protected]>
Signed-off-by: flaviuvadan <[email protected]>
Signed-off-by: flaviuvadan <[email protected]>
Signed-off-by: flaviuvadan <[email protected]>
Signed-off-by: flaviuvadan <[email protected]>
@@ -20,3 +20,4 @@ jobs: | |||
- run: make --directory sdks/${{matrix.name}} publish -B | |||
env: | |||
JAVA_SDK_MAVEN_PASSWORD: ${{ secrets.GITHUB_TOKEN }} | |||
PYPI_API_TOKEN: ${{ secrets.PYPI_API_TOKEN }} |
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.
We don't have this yet but I can add before next release. Could you use your fork and https://test.pypi.org/ to test that the entire release process works?
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.
@terrytangyuan I tested the release process and discovered two bugs:
- secret reference was missing
${}
- the built full
dist
path was missing
I fixed those. During testing I had to name the package argo-workflows-test
because I do not have permission to argo-workflows
. Here are the results:
@terrytangyuan I noticed that there are other similar validation errors happening with other generated objects. For example, I fail to obtain the status of a workflow because of client side validation, and I get this error: instance = klass(**kwargs)
File "/Users/Flaviu_Vadan/.virtualenvs/X-ESYgr3Xo/lib/python3.7/site-packages/argo/workflows/client/models/v1alpha1_gcs_artifact.py", line 58, in __init__
self.bucket = bucket
File "/Users/Flaviu_Vadan/.virtualenvs/X-ESYgr3Xo/lib/python3.7/site-packages/argo/workflows/client/models/v1alpha1_gcs_artifact.py", line 84, in bucket
raise ValueError("Invalid value for `bucket`, must not be `None`") # noqa: E501
ValueError: Invalid value for `bucket`, must not be `None` I did more testing and I think a better solution that the one presented by this PR, as is, is to add the following to the kwargs that are passed to objects upon deserialization: config = Configuration()
config.client_side_validation = False
kwargs['local_vars_configuration'] = config From a quick search, I think every generated object has this block in the if local_vars_configuration is None:
local_vars_configuration = Configuration()
self.local_vars_configuration = local_vars_configuration When # Disable client side validation
self.client_side_validation = True Disabling validation, while not ideal, is certainly what we need to avoid any potential deserialization errors on the client side. The Argo Server might be correct in not sending them sometimes, but the SDK autogenerated code does not seem to know about that. So, I suggest we disable validation, for otherwise we will have to hack multiple fields, not just |
Signed-off-by: flaviuvadan <[email protected]>
Disabling client-side validation sounds good to me as long as we have a note in the documentation to inform users how to turn it on when needed. |
Signed-off-by: flaviuvadan <[email protected]>
@terrytangyuan I did a last test before continuing down this path - I downgraded the OpenAPI generator to 4.3.1 as it used to be here. This solved the deserialization problems and there's no longer a need for a hack. I set This might actually help with: |
Signed-off-by: flaviuvadan <[email protected]>
fix: add Pipfile to gitignore, run pre-commit, fix conflict Signed-off-by: flaviuvadan <[email protected]>
Looks like downgrading is the best option for now. I think it's fine. Did you test the release workflow? Could you point me to the package in test PyPI? |
Will those two linked issues be fixed by downgrading OpenAPI-gen as well? |
Yes, linked in this comment. |
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.
Narrow down the PR to only change release process.
Signed-off-by: flaviuvadan <[email protected]>
This PR adds the necessary PyPi publication steps for the Python SDK
and also adds a script for fixing the SDK deserialization in the Python API client. The OpenAPI generated code does not properly deserialize the response from the Argo Server, ultimately not addingdatetime
objects on the workflow status, causing the following error:The same error is thrown for thefinished_at
field.After much discussion with @terrytangyuan we decided to isolate this PR to SDK publication only. We're going to open a different PR to explore fixes to the autogenerated SDK.
Will need a maintainer to publish to PyPy using tag v6.0.0. Yuan and I discussed this and we're going to keep using https://pypi.org/project/argo-workflows/ but publish the new one as 6.0.0.
Fixes:
#7293 and#7130