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

Proposal to move prefect REST API client to separate package/library #6308

Closed
3 tasks done
xnuinside opened this issue Aug 8, 2022 · 13 comments
Closed
3 tasks done
Labels
enhancement An improvement of an existing feature needs:design Blocked by a need for an implementation outline

Comments

@xnuinside
Copy link

First check

  • I added a descriptive title to this issue.
  • I used the GitHub search to find a similar request and didn't find it.
  • I searched the Prefect documentation for this feature.

Prefect Version

2.x

Describe the proposed behavior

First of all, thanks for the great tool.

This issue is a request to have separate prefect-client library to interact with Prefect REST API. Or split prefect.client from current library.

You call REST API of some server usually from the client, not server itself, and there is no any need to get such huge package as prefect with all server-logic required staff

in some cases it is critical to think about package size and dependencies, for example, like in AWS Lambdas

in will be great to have a light-weight python-client for Prefect REST API without need load all server-dependencies

Because, other way, we need to write our own 'providers'/'adapters' to interact with API to avoid load huge package in dependencies, like here https://github.com/geobeyond/fastflows/blob/develop/fastflows/providers/prefect.py#L50.

If maintainers will be ok with that - we can help with moving REST API client to separate prefect-client library. Ideally it should have in deps only httpx, pydantic, and/or minor deps only for developing proper REST API python client.

Describe the current behavior

install whole prefect package

Example Use

I has AWS Lambda that need to get state of prefect flow.

Or I have a backend server that use prefect API for some feature, or I create a package that interact with different flows managers as an upper layer. For example, https://github.com/geobeyond/fastflows/blob/develop/fastflows/providers/prefect.py#L50

Additional context

No response

@xnuinside xnuinside added enhancement An improvement of an existing feature status:needs-triage labels Aug 8, 2022
@xnuinside xnuinside changed the title Move prefect client to iterate with Prefect REST API in separate library. Move prefect REST API client to separate library Aug 8, 2022
@xnuinside xnuinside changed the title Move prefect REST API client to separate library Move prefect REST API client to separate package/library Aug 8, 2022
@xnuinside xnuinside changed the title Move prefect REST API client to separate package/library Proposal to move prefect REST API client to separate package/library Aug 8, 2022
@zanieb
Copy link
Contributor

zanieb commented Aug 8, 2022

Hi! Thanks for this well written issue. I think your suggestion has a lot of merit, but a change like this is something that we'll need to discuss as a team.

A couple concerns off the top of my head:

  • The client supports connection directly to an in-memory FastAPI application, which means we'd still have that dependency or we'd have to introduce a separate client implementation.
  • The client makes use of many utilities from the core library, some of which we probably do not want to replicate but it'd be weird for the core library to import them from the client implementation.

@zanieb zanieb added the needs:design Blocked by a need for an implementation outline label Aug 8, 2022
@xnuinside
Copy link
Author

xnuinside commented Aug 8, 2022

@madkinsz I also thought about it when I look at source code, so second thought was not to touch current prefect package, but create fresh new prefect-client package based on open api configuration that already exists in prefect (possible to try something with autogeneration from OpenAPI). But anyway will be great to hear team feedback. And thanks for the quick response!

@zanieb
Copy link
Contributor

zanieb commented Aug 9, 2022

I'm going to mark this as deferred for now, as I do not think we can undertake this in the near future.

Loosely, I think that creating a prefect-openapi-client is something we're interested in. I imagine a package like

prefect_openapi_client
    python
       prefect_openapi_client
          ...
       setup.py

with auto-generated clients for different languages from our OpenAPI specifications would be great.

@xnuinside
Copy link
Author

@madkinsz can I ask why still use setup.py + setup.cfg and not modern pyproject.toml & for example, poetry?

@xnuinside
Copy link
Author

there is a bunch of issues in Prefect open api config
Screenshot 2022-08-11 at 14 19 16
need to fix them firstly to make autogeneration works )

for example:

Screenshot 2022-08-11 at 14 16 11

in Open Api 3.0 'null' is not a type but it is defined as a response type in schema in endpoints /deployments/{id}/set_schedule_inactive & /deployments/{id}/set_schedule_active

@zanieb
Copy link
Contributor

zanieb commented Aug 11, 2022

We don't have internal consensus on alternative package manager to use an pip / setuptools has all the features we need.

What are you using to detect the OpenAPI errors?

@xnuinside
Copy link
Author

setuptools also support pyproject toml https://setuptools.pypa.io/en/latest/userguide/pyproject_config.html ) setup.cfg & setup.py is pretty old & not actual since PEP, pip also support pyproject.toml https://pip.pypa.io/en/stable/reference/build-system/pyproject-toml/

but anyway

about errors - I tried generator - https://github.com/openapi-generators/openapi-python-client

Full list OpenApi issues:


Failed to parse OpenAPI document

9 validation errors for OpenAPI
paths -> /deployments/{id}/schedule -> post -> responses -> 200 -> content -> application/json -> schema -> $ref
  field required (type=value_error.missing)
paths -> /deployments/{id}/schedule -> post -> responses -> 200 -> content -> application/json -> schema -> type
  value is not a valid enumeration member; permitted: 'string', 'number', 'integer', 'boolean', 'array', 'object' (type=type_error.enum; enum_values=[<DataType.STRING: 'string'>, <DataType.NUMBER: 'number'>, <DataType.INTEGER: 'integer'>, <DataType.BOOLEAN: 'boolean'>, <DataType.ARRAY: 'array'>, <DataType.OBJECT: 'object'>])
paths -> /deployments/{id}/schedule -> post -> responses -> 200 -> $ref
  field required (type=value_error.missing)
paths -> /deployments/{id}/set_schedule_active -> post -> responses -> 200 -> content -> application/json -> schema -> $ref
  field required (type=value_error.missing)
paths -> /deployments/{id}/set_schedule_active -> post -> responses -> 200 -> content -> application/json -> schema -> type
  value is not a valid enumeration member; permitted: 'string', 'number', 'integer', 'boolean', 'array', 'object' (type=type_error.enum; enum_values=[<DataType.STRING: 'string'>, <DataType.NUMBER: 'number'>, <DataType.INTEGER: 'integer'>, <DataType.BOOLEAN: 'boolean'>, <DataType.ARRAY: 'array'>, <DataType.OBJECT: 'object'>])
paths -> /deployments/{id}/set_schedule_active -> post -> responses -> 200 -> $ref
  field required (type=value_error.missing)
paths -> /deployments/{id}/set_schedule_inactive -> post -> responses -> 200 -> content -> application/json -> schema -> $ref
  field required (type=value_error.missing)
paths -> /deployments/{id}/set_schedule_inactive -> post -> responses -> 200 -> content -> application/json -> schema -> type
  value is not a valid enumeration member; permitted: 'string', 'number', 'integer', 'boolean', 'array', 'object' (type=type_error.enum; enum_values=[<DataType.STRING: 'string'>, <DataType.NUMBER: 'number'>, <DataType.INTEGER: 'integer'>, <DataType.BOOLEAN: 'boolean'>, <DataType.ARRAY: 'array'>, <DataType.OBJECT: 'object'>])
paths -> /deployments/{id}/set_schedule_inactive -> post -> responses -> 200 -> $ref
  field required (type=value_error.missing)

all of them correct in scope current OpenAPI 3.x standard

@xnuinside
Copy link
Author

xnuinside commented Aug 11, 2022

@madkinsz also about setuptools & setup.cfg - pypa/setuptools#3214 they plan to deprecate it & milestone for it https://github.com/pypa/setuptools/milestone/7

but setup.py is used for sure ) question only in setup.cfg

@zanieb
Copy link
Contributor

zanieb commented Aug 12, 2022

about errors - I tried generator - https://github.com/openapi-generators/openapi-python-client

Thanks! @abrookins should we open an issue to ensure our OpenAPI specification conforms to the latest standards and include some sort of test?

also about setuptools & setup.cfg - pypa/setuptools#3214 they plan to deprecate it & milestone for it https://github.com/pypa/setuptools/milestone/7

Yeah but they haven't deprecated it yet and they noting that they want to wait for pyproject.toml to stabilize first. This isn't really affecting our developer experience and we'd rather have a stable and familiar config for now. We are likely to move to the new formats sometime in the future.

@abrookins
Copy link
Collaborator

@madkinsz Yes, let's open an issue to sort out what's happening here. We should be able to create a unit test that validates the output of our OpenAPI spec against the supported version (3.0.2 AFAIK, from FastAPI).

@abrookins
Copy link
Collaborator

I created #6400 to track validating our OpenAPI spec.

@github-actions
Copy link
Contributor

This issue is stale because it has been open 30 days with no activity. To keep this issue open remove stale label or comment.

@github-actions
Copy link
Contributor

This issue was closed because it has been stale for 7 days with no activity. If this issue is important or you have more to add feel free to re-open it.

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Sep 28, 2022
@zanieb zanieb mentioned this issue Oct 4, 2022
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement An improvement of an existing feature needs:design Blocked by a need for an implementation outline
Projects
None yet
Development

No branches or pull requests

3 participants