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

Flytekit add async agent test, fix delete task agent service and fix python3.8 asyncio to_thread issue #1848

Conversation

Future-Outlier
Copy link
Member

@Future-Outlier Future-Outlier commented Sep 23, 2023

TL;DR

python3.8 doesn't have the function asyncio.to_thread, we need to use asyncio.run_in_executor instead.

Type

  • Bug Fix
  • Feature
  • Plugin

Are all requirements met?

  • Code completed
  • Smoke tested
  • Unit tests added
  • Code documentation added
  • Any pending items have an associated Issue

Complete description

Since the file agent-service.py has changed, the test needs to be changed too.

Tracking Issue

flyteorg/flyte#3936

Screenshot

image

pingsutw
pingsutw previously approved these changes Sep 23, 2023
@codecov
Copy link

codecov bot commented Sep 23, 2023

Codecov Report

Attention: 1371 lines in your changes are missing coverage. Please review.

Comparison is base (3370a96) 71.03% compared to head (4617024) 54.96%.
Report is 116 commits behind head on master.

❗ Current head 4617024 differs from pull request most recent head 660ae57. Consider uploading reports for the commit 660ae57 to get more accurate results

Additional details and impacted files
@@             Coverage Diff             @@
##           master    #1848       +/-   ##
===========================================
- Coverage   71.03%   54.96%   -16.08%     
===========================================
  Files         336      296       -40     
  Lines       30798    22216     -8582     
  Branches     5589     3337     -2252     
===========================================
- Hits        21876    12210     -9666     
- Misses       8375     9856     +1481     
+ Partials      547      150      -397     
Files Coverage Δ
flytekit/clients/friendly.py 49.65% <ø> (+44.75%) ⬆️
flytekit/configuration/file.py 57.95% <100.00%> (+3.97%) ⬆️
flytekit/configuration/internal.py 81.01% <100.00%> (+65.01%) ⬆️
flytekit/core/constants.py 100.00% <100.00%> (+100.00%) ⬆️
flytekit/core/dynamic_workflow_task.py 100.00% <ø> (+100.00%) ⬆️
flytekit/core/gate.py 37.50% <100.00%> (-11.90%) ⬇️
flytekit/core/task.py 66.03% <100.00%> (+37.19%) ⬆️
flytekit/experimental/__init__.py 100.00% <100.00%> (ø)
flytekit/extend/__init__.py 100.00% <ø> (ø)
flytekit/extras/sqlite3/task.py 0.00% <ø> (-90.75%) ⬇️
... and 105 more

... and 360 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Future-Outlier
Copy link
Member Author

Future-Outlier commented Sep 23, 2023

I haven't run pip compile for the requirements, please wait for it and I will update the dependency!
Update: I think the GitHub action will do it for me, it can be approved

Copy link
Member

@pingsutw pingsutw left a comment

Choose a reason for hiding this comment

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

Tests are failing in python 3.8, could you take a look

@Future-Outlier
Copy link
Member Author

Tests are failing in python 3.8, could you take a look

No problem!

@Future-Outlier
Copy link
Member Author

Future-Outlier commented Sep 25, 2023

@pingsutw I think the main reason is under here.
https://docs.python.org/3/whatsnew/3.9.html
image
Asyncio to_thread function is not available on python3.8, but we use it in our code.

image

@Future-Outlier
Copy link
Member Author

Just update the code, please review, thanks a lot!

@Future-Outlier Future-Outlier changed the title Flytekit add async agent test and fix do task agent service Flytekit add async agent test, fix do task agent service and fix python3.8 asyncio to_thread issue Sep 27, 2023
@Future-Outlier Future-Outlier changed the title Flytekit add async agent test, fix do task agent service and fix python3.8 asyncio to_thread issue Flytekit add async agent test, fix delete task agent service and fix python3.8 asyncio to_thread issue Sep 29, 2023
eapolinario and others added 14 commits October 3, 2023 13:49
* Test experimental map task so far

Signed-off-by: eduardo apolinario <[email protected]>

* Lint

Signed-off-by: eduardo apolinario <[email protected]>

* Include ArrayNodeMapTask task spec in list of registrable entities

Signed-off-by: eduardo apolinario <[email protected]>

* can start fast-register

Signed-off-by: Daniel Rammer <[email protected]>

* fixed task type

Signed-off-by: Daniel Rammer <[email protected]>

* Add _execute_map_task

Signed-off-by: eduardo apolinario <[email protected]>

* added self to _compute_array_job_index function

Signed-off-by: Daniel Rammer <[email protected]>

* overridding _outputs_interface

Signed-off-by: Daniel Rammer <[email protected]>

* imports

Signed-off-by: Daniel Rammer <[email protected]>

* added get_type_for_output_var override

Signed-off-by: Daniel Rammer <[email protected]>

* using correct python outputs type

Signed-off-by: Daniel Rammer <[email protected]>

* not appending task index to output_prefix for ArrayNode

Signed-off-by: Daniel Rammer <[email protected]>

* using metadata from subtasks

Signed-off-by: Daniel Rammer <[email protected]>

* updated task interface

Signed-off-by: Daniel Rammer <[email protected]>

* reverted interface - will need to handle in propeller compiler

Signed-off-by: Daniel Rammer <[email protected]>

* Hash metadata

* Fix bindings to ArrayNode

Signed-off-by: eduardo apolinario <[email protected]>

* Minor cleanup

Signed-off-by: eduardo apolinario <[email protected]>

* Remove extraneous code

Signed-off-by: eduardo apolinario <[email protected]>

* Use correct version of flyteidl

Signed-off-by: eduardo apolinario <[email protected]>

* More cleanup

Signed-off-by: eduardo apolinario <[email protected]>

* Use experimental module

Signed-off-by: eduardo apolinario <[email protected]>

* Fix tests

Signed-off-by: eduardo apolinario <[email protected]>

* Add support for partials

Signed-off-by: eduardo apolinario <[email protected]>

* Add support for fast registration

Signed-off-by: eduardo apolinario <[email protected]>

* Linting

Signed-off-by: eduardo apolinario <[email protected]>

* Remove a few TODOs

Signed-off-by: eduardo apolinario <[email protected]>

---------

Signed-off-by: eduardo apolinario <[email protected]>
Signed-off-by: Daniel Rammer <[email protected]>
Signed-off-by: Eduardo Apolinario <[email protected]>
Co-authored-by: eduardo apolinario <[email protected]>
Co-authored-by: Daniel Rammer <[email protected]>
Signed-off-by: Future Outlier <[email protected]>
…teorg#1782)

* reupload the agent-ctrl-c handler and grpc dependency

Signed-off-by: Future Outlier <[email protected]>

* image config annotation

Signed-off-by: Future Outlier <[email protected]>

* Trigger RTD build

Signed-off-by: Future Outlier <[email protected]>

* update grpc dependency (correct version)

Signed-off-by: Future Outlier <[email protected]>

---------

Signed-off-by: Future Outlier <[email protected]>
Co-authored-by: Future Outlier <[email protected]>
Signed-off-by: Future Outlier <[email protected]>
* wip

Signed-off-by: Kevin Su <[email protected]>

* Add tests

Signed-off-by: Kevin Su <[email protected]>

* Use JsonParamType instead

Signed-off-by: Kevin Su <[email protected]>

* update

Signed-off-by: Kevin Su <[email protected]>

* update idl

Signed-off-by: Kevin Su <[email protected]>

* update idl

Signed-off-by: Kevin Su <[email protected]>

* update idl

Signed-off-by: Kevin Su <[email protected]>

* update idl

Signed-off-by: Kevin Su <[email protected]>

* bump grpcio-status version

Signed-off-by: Kevin Su <[email protected]>

* bump grpcioversion

Signed-off-by: Kevin Su <[email protected]>

---------

Signed-off-by: Kevin Su <[email protected]>
Co-authored-by: Eduardo Apolinario <[email protected]>
Signed-off-by: Future Outlier <[email protected]>
Signed-off-by: troychiu <[email protected]>
Signed-off-by: Future Outlier <[email protected]>
Signed-off-by: Future Outlier <[email protected]>
Co-authored-by: Future Outlier <[email protected]>
Signed-off-by: Future Outlier <[email protected]>
* Beautified pyflyte run even for every task and workflow

- identify a task or a workflow
- task or workflow help menus show types and use rich to beautify

Signed-off-by: Ketan Umare <[email protected]>

* one more improvement

Signed-off-by: Ketan Umare <[email protected]>

---------

Signed-off-by: Ketan Umare <[email protected]>
Signed-off-by: Future Outlier <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Future Outlier <[email protected]>
* Add type hint for base_tas.py compile method

Signed-off-by: Future Outlier <[email protected]>

* add-type-annotations-for-tensorflow_model_test_transformation

Signed-off-by: Future Outlier <[email protected]>

* Add 2 type annotations in configuration/file.py

Signed-off-by: Future Outlier <[email protected]>

* revert test_transformations.py annotations

Signed-off-by: Future Outlier <[email protected]>

---------

Signed-off-by: Future Outlier <[email protected]>
Co-authored-by: Future Outlier <[email protected]>
Signed-off-by: Future Outlier <[email protected]>
…rg#1791)

* Add type hint for base_tas.py compile method

Signed-off-by: Future Outlier <[email protected]>

* add-type-annotations-for-tensorflow_model_test_transformation

Signed-off-by: Future Outlier <[email protected]>

---------

Signed-off-by: Future Outlier <[email protected]>
Co-authored-by: Future Outlier <[email protected]>
Signed-off-by: Future Outlier <[email protected]>
)

* Add Azure-specific headers when uploading to blob storage

Signed-off-by: Victor Delépine <[email protected]>

* Add comment about HTTP 201 check

Signed-off-by: Victor Delépine <[email protected]>

---------

Signed-off-by: Victor Delépine <[email protected]>
Signed-off-by: Future Outlier <[email protected]>
Signed-off-by: Future Outlier <[email protected]>
Co-authored-by: Future Outlier <[email protected]>
Signed-off-by: Future Outlier <[email protected]>
Signed-off-by: troychiu <[email protected]>
Signed-off-by: Future Outlier <[email protected]>
Signed-off-by: Yee Hing Tong <[email protected]>
Signed-off-by: Future Outlier <[email protected]>
…json for improved static type checking (flyteorg#1801)

* Inherit directly from DataClassJsonMixin instead of @dataclass_json for improved static type checking

As it says in the dataclasses-json README: https://github.com/lidatong/dataclasses-json/blob/89578cb9ebed290e70dba8946bfdb68ff6746755/README.md?plain=1#L111-L129, we can use inheritance for improved static type checking; this one change eliminates something like 467 pyright errors from the flytekit module

Signed-off-by: Matthew Hoffman <[email protected]>
Signed-off-by: Future Outlier <[email protected]>
pingsutw and others added 27 commits October 3, 2023 13:49
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Future Outlier <[email protected]>
* implement eager workflow local entrypoint, support offloaded types

Signed-off-by: Niels Bantilan <[email protected]>

* wip local entrypoint

Signed-off-by: Niels Bantilan <[email protected]>

* add tests

Signed-off-by: Niels Bantilan <[email protected]>

* add local entrypoint tests

Signed-off-by: Niels Bantilan <[email protected]>

* update eager unit tests, delete test script

Signed-off-by: Niels Bantilan <[email protected]>

* clean up tests

Signed-off-by: Niels Bantilan <[email protected]>

* update ci

Signed-off-by: Niels Bantilan <[email protected]>

* update ci

Signed-off-by: Niels Bantilan <[email protected]>

* update ci

Signed-off-by: Niels Bantilan <[email protected]>

* update ci

Signed-off-by: Niels Bantilan <[email protected]>

* remove push step

Signed-off-by: Niels Bantilan <[email protected]>

---------

Signed-off-by: Niels Bantilan <[email protected]>
Signed-off-by: Future Outlier <[email protected]>
…g#1838)

* update requirements and add snowflake agent to api reference

Signed-off-by: Samhita Alla <[email protected]>

* update requirements

Signed-off-by: Samhita Alla <[email protected]>

* remove versions

Signed-off-by: Samhita Alla <[email protected]>

* remove tensorflow-macos

Signed-off-by: Samhita Alla <[email protected]>

* lint

Signed-off-by: Samhita Alla <[email protected]>

* downgrade sphinxcontrib-youtube package

Signed-off-by: Samhita Alla <[email protected]>

---------

Signed-off-by: Samhita Alla <[email protected]>
Signed-off-by: Future Outlier <[email protected]>
…to parent process (flyteorg#1837)

* Transfer decks created in the worker process to the parent process

Signed-off-by: Fabio Graetz <[email protected]>

* Add test for decks in elastic tasks

Signed-off-by: Fabio Graetz <[email protected]>

* Update plugins/flytekit-kf-pytorch/flytekitplugins/kfpytorch/task.py

Signed-off-by: Fabio Graetz <[email protected]>

* Update plugins/flytekit-kf-pytorch/flytekitplugins/kfpytorch/task.py

Signed-off-by: Fabio Graetz <[email protected]>

---------

Signed-off-by: Fabio Graetz <[email protected]>
Signed-off-by: Future Outlier <[email protected]>
* add accept grpc

Signed-off-by: Yee Hing Tong <[email protected]>
Signed-off-by: Jeev B <[email protected]>

* unpin setup.py grpc

Signed-off-by: Yee Hing Tong <[email protected]>
Signed-off-by: Jeev B <[email protected]>

* Revert "add accept grpc"

This reverts commit 2294592.

Signed-off-by: Jeev B <[email protected]>

* default headers interceptor

Signed-off-by: Jeev B <[email protected]>

* setup.py

Signed-off-by: Jeev B <[email protected]>

* fixes

Signed-off-by: Jeev B <[email protected]>

* fmt

Signed-off-by: Jeev B <[email protected]>

* move prometheus-client import

Signed-off-by: Jeev B <[email protected]>

---------

Signed-off-by: Yee Hing Tong <[email protected]>
Signed-off-by: Jeev B <[email protected]>
Co-authored-by: Jeev B <[email protected]>
Signed-off-by: Future Outlier <[email protected]>
…dmin (flyteorg#1787)

* Introduce authenticator engine and make proxy auth work

Signed-off-by: Fabio Grätz <[email protected]>

* Use proxy authed session for client credentials flow

Signed-off-by: Fabio Grätz <[email protected]>

* Don't use authenticator engine but do proxy authentication via existing external command authenticator

Signed-off-by: Fabio Grätz <[email protected]>

* Add docstring to AuthenticationHTTPAdapter

Signed-off-by: Fabio Grätz <[email protected]>

* Address todo in docstring

Signed-off-by: Fabio Grätz <[email protected]>

* Create blank session if none provided

Signed-off-by: Fabio Grätz <[email protected]>

* Create blank session if none provided in get_token

Signed-off-by: Fabio Grätz <[email protected]>

* Refresh proxy creds in session when not existing without triggering 401

Signed-off-by: Fabio Grätz <[email protected]>

* Add test for get_session

Signed-off-by: Fabio Grätz <[email protected]>

* Move auth helper test into existing module

Signed-off-by: Fabio Grätz <[email protected]>

* Move auth helper test into existing module

Signed-off-by: Fabio Grätz <[email protected]>

* Add test for upgrade_channel_to_proxy_authenticated

Signed-off-by: Fabio Grätz <[email protected]>

* Auth helper tests without use of responses package

Signed-off-by: Fabio Grätz <[email protected]>

* Feat: Add plugin for generating GCP IAP ID tokens via external command (flyteorg#1795)

* Add external command plugin to generate id tokens for identity aware proxy

Signed-off-by: Fabio Grätz <[email protected]>

* Retrieve desktop app client secret from gcp secret manager

Signed-off-by: Fabio Grätz <[email protected]>

* Remove comments

Signed-off-by: Fabio Grätz <[email protected]>

* Introduce a command group that allows adding a command to generate service account id tokens later

Signed-off-by: Fabio Grätz <[email protected]>

* Document how to use plugin and deploy Flyte with IAP

Signed-off-by: Fabio Grätz <[email protected]>

* Minor corrections README.md

Signed-off-by: Fabio Grätz <[email protected]>

---------

Signed-off-by: Fabio Grätz <[email protected]>
Co-authored-by: Fabio Grätz <[email protected]>
Signed-off-by: Fabio Grätz <[email protected]>

* Use proxy auth'ed session for device code auth flow

Signed-off-by: Fabio Grätz <[email protected]>

* Fix token client tests

Signed-off-by: Fabio Grätz <[email protected]>

* Make poll token endpoint test more specific

Signed-off-by: Fabio Grätz <[email protected]>

* Make test_client_creds_authenticator test work and more specific

Signed-off-by: Fabio Grätz <[email protected]>

* Make test_client_creds_authenticator_with_custom_scopes test work and more specific

Signed-off-by: Fabio Grätz <[email protected]>

* Implement subcommand to generate id tokens for service accounts

Signed-off-by: Fabio Graetz <[email protected]>

* Test id token generation from service accounts

Signed-off-by: Fabio Graetz <[email protected]>

* Fix plugin requirements

Signed-off-by: Fabio Graetz <[email protected]>

* Document usage of generate-service-account-id-token subcommand

Signed-off-by: Fabio Grätz <[email protected]>

* Document alternative ways to obtain service account id tokens

Signed-off-by: Fabio Grätz <[email protected]>

---------

Signed-off-by: Fabio Grätz <[email protected]>
Signed-off-by: Fabio Graetz <[email protected]>
Co-authored-by: Fabio Grätz <[email protected]>
Signed-off-by: Future Outlier <[email protected]>
* Pass cluster pool when creating executions

Signed-off-by: Future Outlier <[email protected]>
* add more clear error  message when fetch secrets

Signed-off-by: Yue Shang <[email protected]>

* fix format

Signed-off-by: Yue Shang <[email protected]>

---------

Signed-off-by: Yue Shang <[email protected]>
Signed-off-by: Future Outlier <[email protected]>
Signed-off-by: Future Outlier <[email protected]>
…rg#1849)

* fix pyflyte run handling of default None

Signed-off-by: Niels Bantilan <[email protected]>

* update tests

Signed-off-by: Niels Bantilan <[email protected]>

* remote print statements in tests

Signed-off-by: Niels Bantilan <[email protected]>

---------

Signed-off-by: Niels Bantilan <[email protected]>
Signed-off-by: Future Outlier <[email protected]>
Bumps [gitpython](https://github.com/gitpython-developers/GitPython) from 3.1.32 to 3.1.35.
- [Release notes](https://github.com/gitpython-developers/GitPython/releases)
- [Changelog](https://github.com/gitpython-developers/GitPython/blob/main/CHANGES)
- [Commits](gitpython-developers/GitPython@3.1.32...3.1.35)

---
updated-dependencies:
- dependency-name: gitpython
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Signed-off-by: Future Outlier <[email protected]>
Bumps [cryptography](https://github.com/pyca/cryptography) from 41.0.3 to 41.0.4.
- [Changelog](https://github.com/pyca/cryptography/blob/main/CHANGELOG.rst)
- [Commits](pyca/cryptography@41.0.3...41.0.4)

---
updated-dependencies:
- dependency-name: cryptography
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Signed-off-by: Future Outlier <[email protected]>
* update codecov config in pythonbuild ci

Signed-off-by: Niels Bantilan <[email protected]>

* updates

Signed-off-by: Niels Bantilan <[email protected]>

---------

Signed-off-by: Niels Bantilan <[email protected]>
Signed-off-by: Future Outlier <[email protected]>
Signed-off-by: Ketan Umare <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Co-authored-by: Kevin Su <[email protected]>
Signed-off-by: Future Outlier <[email protected]>
* update codecov yaml, make eager wf test more stable

Signed-off-by: Niels Bantilan <[email protected]>

* ignore plugin setup.py

Signed-off-by: Niels Bantilan <[email protected]>

* update codecov

Signed-off-by: Niels Bantilan <[email protected]>

* ignore clis

Signed-off-by: Niels Bantilan <[email protected]>

---------

Signed-off-by: Niels Bantilan <[email protected]>
Signed-off-by: Future Outlier <[email protected]>
* more codecov updates

Signed-off-by: Niels Bantilan <[email protected]>

* fix

Signed-off-by: Niels Bantilan <[email protected]>

---------

Signed-off-by: Niels Bantilan <[email protected]>
Signed-off-by: Future Outlier <[email protected]>
Signed-off-by: Bram van Meurs <[email protected]>
Signed-off-by: Future Outlier <[email protected]>
…nto flytekit-fix-do-task-agent-service

Signed-off-by: Future Outlier <[email protected]>
Signed-off-by: Future Outlier <[email protected]>
Co-authored-by: Future Outlier <[email protected]>
Signed-off-by: Future Outlier <[email protected]>
Signed-off-by: Thomas Newton <[email protected]>
Signed-off-by: Future Outlier <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Future Outlier <[email protected]>
Signed-off-by: HH <[email protected]>
Signed-off-by: Future Outlier <[email protected]>
---------

Signed-off-by: Edwin Yu <[email protected]>
Co-authored-by: Edwin Yu <[email protected]>
Signed-off-by: Future Outlier <[email protected]>
---------

Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Future Outlier <[email protected]>
Signed-off-by: Future Outlier <[email protected]>
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.