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

generate python constructor types for s3 services #5451

Closed
wants to merge 4 commits into from

Conversation

trim21
Copy link
Contributor

@trim21 trim21 commented Dec 24, 2024

Which issue does this PR close?

Closes #.

Rationale for this change

Not sure if this is a good idea for opendal developers or not, this add extra burden in keeping types synced with builder.

What changes are included in this PR?

add type annotation for s3 operator.

Are there any user-facing changes?

Yes, add argument typing checking for users using s3 service.

@trim21 trim21 changed the title add python types for some services add python types for s3 services Dec 24, 2024
@trim21 trim21 changed the title add python types for s3 services add python contractor types for s3 services Dec 24, 2024
@messense messense changed the title add python contractor types for s3 services add python constructor types for s3 services Dec 25, 2024
Copy link
Member

@messense messense left a comment

Choose a reason for hiding this comment

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

It's a nice addition IMO, but it does add more maintenance burden. I'd be happy to include it if we can add a pytest to at least validate it to some extend, like it has specified all possible arguments. But it does not seem easy to do because the Python binding api does not expose any information about the config of each service.

Maybe we could also consider exposing XXXConfig to python and also accept that in Operator(config=XXXConfig()) so it's strongly typed?

@trim21
Copy link
Contributor Author

trim21 commented Dec 25, 2024

like it has specified all possible arguments

Some arguments are optional, I don't think this is possible?

Maybe we could also consider exposing XXXConfig to python and also accept that in Operator(config=XXXConfig()) so it's strongly typed?

Then how we type XXXConfig.__init__?

@messense
Copy link
Member

messense commented Dec 25, 2024

To clarify, I meant all possible config arguments are included in .pyi.

Then how we type XXXConfig.__init__?

Just like any other #[pyclass], it'll be easy to validate because we can use ast to parse .pyi while using inspect module to get argspec of the #[pyclass] constructor, then you can compare them in test case.

@trim21
Copy link
Contributor Author

trim21 commented Dec 25, 2024

To clarify, I meant all possible config arguments are included in .pyi.

Then how we type XXXConfig.__init__?

Just like any other #[pyclass], it'll be easy to validate because we can use ast to parse .pyi while using inspect module to get argspec of the #[pyclass] constructor, then you can compare them in test case.

in that case,I think we can generate Operator.init overloads from config directly?

@messense
Copy link
Member

I think we can generate Operator.init overloads from config directly?

Totally

@trim21 trim21 marked this pull request as draft December 25, 2024 01:54
@trim21
Copy link
Contributor Author

trim21 commented Dec 25, 2024

I'll have a try

@Xuanwo
Copy link
Member

Xuanwo commented Dec 25, 2024

Node.js, C++, and Java bindings all have similar issues. I'm wondering if there is a general solution for this.

@trim21 trim21 changed the title add python constructor types for s3 services generate python constructor types for s3 services Dec 25, 2024
@trim21 trim21 marked this pull request as ready for review December 25, 2024 03:57
@trim21 trim21 requested review from Xuanwo and PsiACE as code owners December 25, 2024 03:57
@trim21
Copy link
Contributor Author

trim21 commented Dec 25, 2024

I don't know rust very well, so I write a very simple rust script to generat pyi for s3.

I'm guessing maybe most of these rust code can be improved.

I do not know internal details about opendal., but looks like S3Config doesn' match options in docs https://opendal.apache.org/docs/rust/opendal/services/struct.S3.html

@Xuanwo
Copy link
Member

Xuanwo commented Dec 25, 2024

Hi, I like this direction. I believe we can integrate it into our workflow more effectively. Let me give it a try.

@Xuanwo
Copy link
Member

Xuanwo commented Dec 25, 2024

Hi, @trim21. I plan to add some helper utilities to our dev tools. Would you be interested in adding a Python code generator to it later? I'll ping you on the PR.

@trim21
Copy link
Contributor Author

trim21 commented Dec 25, 2024

Hi, @trim21. I plan to add some helper utilities to our dev tools. Would you be interested in adding a Python code generator to it later? I'll ping you on the PR.

ok

@trim21
Copy link
Contributor Author

trim21 commented Dec 25, 2024

oops, didn't notice there is already a empty dev crate.

@trim21
Copy link
Contributor Author

trim21 commented Dec 25, 2024

I think we should generate some kind of ir and generate types for each languages from ir

@Xuanwo
Copy link
Member

Xuanwo commented Dec 25, 2024

I think we should generate some kind of ir and generate types for each languages from ir

100%!

Working on this now.

@trim21
Copy link
Contributor Author

trim21 commented Dec 25, 2024

I think we should generate some kind of ir and generate types for each languages from ir

100%!

Working on this now.

I'll close this for now and wait for your pr. feel free to cherry pick code if you like

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.

3 participants