Skip to content

Conversation

@mattwelke
Copy link
Contributor

No description provided.

@mattwelke mattwelke marked this pull request as ready for review July 17, 2022 03:38
Copy link
Member

@rabbah rabbah left a comment

Choose a reason for hiding this comment

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

LGTM do you mind also updating the travis file so we publish this image nighly as well?

Add Python 3.10
Add Python 3.10
@mattwelke
Copy link
Contributor Author

Sure. I've done that now.

I think my approach here is adding a lot of repeated code, but I saw that the current state of the repo already has code repeated this way (between 3.7 and 3.9) so I figured it wasn't a regression. In the future, maybe we could look for opportunities to DRY it. I saw the Node.js runtime repo DRY'd pretty well like this. Here, I'm sure there are opportunities like creating a base Dockerfile to use, since the only thing changing is the version of the base Python image used.

Maybe we could try to DRY the code during a 3.11 release. Or, if you'd rather keep refactors separate from new features, it could be its own task.

Another thing I think could be improved in the runtime, also either during a DRY refactor, or as its own new feature, is adding support for async main functions. My testing so far showed it didn't work:

  "2022-07-17T01:46:57.046916Z    stderr: File \"/usr/local/lib/python3.10/json/encoder.py\", line 179, in default",
  "2022-07-17T01:46:57.046919Z    stderr: raise TypeError(f'Object of type {o.__class__.__name__} '",
  "2022-07-17T01:46:57.046923Z    stderr: TypeError: Object of type coroutine is not JSON serializable",
  "2022-07-17T01:46:57.049569Z    stderr: sys:1: RuntimeWarning: coroutine 'main' was never awaited",
  "2022-07-17T01:46:57.059Z       stderr: The action did not initialize or run as expected. Log data might be missing."

@mattwelke
Copy link
Contributor Author

Something that still might be left to do is add support for the new "kind" since a new version of each programming language results in a new kind. I remember reading a guide a while ago about creating new runtimes and it mentioned adding your runtime to a list of kinds by updating a .scala file in a central repository. Is there a change like that we'd have to make too? If so, which repo and which file?

@dgrove-oss dgrove-oss merged commit 5e1bded into apache:master Aug 7, 2022
@ningyougang
Copy link
Contributor

ningyougang commented Aug 9, 2022

@mattwelke , after merged this pr into master, seems current master's travis ci's test cases failed: #132, and the test case failed only happened on action-python-v3.10 image

Can you check it? i am also checking it.
I found the reason, i will open a pr soon.
Fixed by this pr: #133

@mattwelke
Copy link
Contributor Author

@ningyougang Sorry, I missed that when I was creating this PR. I originally did a custom 3.10 build by changing a few files in my fork and then using it. All local. It passed /init and /run no problem. This issue must have only applied to the way this repo was set up. Thanks for catching it.

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.

4 participants