Skip to content

Ipy v2 #15

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

Merged
merged 9 commits into from
Jan 29, 2025
Merged

Ipy v2 #15

merged 9 commits into from
Jan 29, 2025

Conversation

chenkasirer
Copy link
Member

@chenkasirer chenkasirer commented Sep 5, 2024

Yet another copy of the componentizer script, this one is almost identical with the new CPython one but compiles components as for the new IronPython interpreter of Rhino8.

Necessary changes are visible in 42a8ef4

@9and3 couldn't add you as reviewer, but would be great if you could have a look.

One thing I noticed is Rhino8 seems to be taking the input/output argument names from the metadata and replaces the RunScript args with them. This means that if argument names are different in the script itself (e.g. Centerline vs. centerline) the script will be broken..
It would be nice not to have to change components to match, but I currently don't see any other way..

Copy link
Member

@gonzalocasas gonzalocasas left a comment

Choose a reason for hiding this comment

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

I think we need a change on action.yml to support this new "interpreter" mode, and the corresponding updates on the readme file, other than that, it lgtm

Copy link
Contributor

@9and3 9and3 left a comment

Choose a reason for hiding this comment

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

Lookin good @chenkasirer thanks for tag me! 🌠 just a tiny missing datatype for RV8.
As mentioned by @gonzalocasas the action (and readme) should be updated to have an additional interpreter otpion where in the workflow there would be:

- uses: compas-dev/compas-actions.ghpython_components@v5
        with:
          source: components
          target: build
          interpreter: ipyV8  # or something like this

And the action.yml would have just another case:

- name: Launch componentizer
run: |
if ("${{ inputs.interpreter }}" -eq "cpython") {
$command="python"
$componentizer="${{ github.action_path }}/componentize_cpy.py"
} else {
$command="ipy"
$componentizer="${{ github.action_path }}/componentize_ipy.py"
}
$params=$componentizer, "${{ inputs.source }}", "${{ inputs.target }}", "--ghio", "./lib"
$prefix="${{ inputs.prefix }}"
if( $prefix )
{
$params=$params + "--prefix", "$prefix"
}
& $command $params
shell: pwsh

Comment on lines 51 to 52
brep="2ceb0405-fdfe-403d-a4d6-8786da45fb9d",
geometrybase="c37956f4-d39c-49c7-af71-1e87f8031b26",
Copy link
Contributor

Choose a reason for hiding this comment

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

missing cloud datatype for RV8
pointcloud="d73c9fb0-365d-458f-9fb5-f4141399311f",

Comment on lines +318 to +326
# this is new in Rhino8, istead of setting the code as a string, we set it as a base64 blob
script = ghpython_root.CreateChunk("Script")
code_base64 = base64.b64encode(code.encode("utf-8"))
code_base64 = str(code_base64)
script.SetString("Text", code_base64)
script.SetString("Title", "IPy2")
language_spec = script.CreateChunk("LanguageSpec")
language_spec.SetString("Taxon", "*.ironpython.python")
language_spec.SetString("Version", "2.*")
Copy link
Contributor

Choose a reason for hiding this comment

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

super nice!

@chenkasirer
Copy link
Member Author

Lookin good @chenkasirer thanks for tag me! 🌠 just a tiny missing datatype for RV8. As mentioned by @gonzalocasas the action (and readme) should be updated to have an additional interpreter otpion where in the workflow there would be:

- uses: compas-dev/compas-actions.ghpython_components@v5
        with:
          source: components
          target: build
          interpreter: ipyV8  # or something like this

And the action.yml would have just another case:

- name: Launch componentizer
run: |
if ("${{ inputs.interpreter }}" -eq "cpython") {
$command="python"
$componentizer="${{ github.action_path }}/componentize_cpy.py"
} else {
$command="ipy"
$componentizer="${{ github.action_path }}/componentize_ipy.py"
}
$params=$componentizer, "${{ inputs.source }}", "${{ inputs.target }}", "--ghio", "./lib"
$prefix="${{ inputs.prefix }}"
if( $prefix )
{
$params=$params + "--prefix", "$prefix"
}
& $command $params
shell: pwsh

@9and3 thanks so much for taking a look!
I missed a bunch of guids that changes etc, but I think I got it right now.

What do we think about ipy_v2 instead of ipyV8? I tend to like ipy_v2 better..

@chenkasirer
Copy link
Member Author

@gonzalocasas @9and3 btw in the course of making this I made a small script which unpacks a compiled component and writes it as XML + code as python. it was really helpful to find out what I was doing wrong.

Not sure where to put it so made a gist: https://gist.github.com/chenkasirer/986edeb23f8e9ce6cf34fb0d4eb7ff9e

@gonzalocasas
Copy link
Member

@gonzalocasas @9and3 btw in the course of making this I made a small script which unpacks a compiled component and writes it as XML + code as python. it was really helpful to find out what I was doing wrong.

Not sure where to put it so made a gist: https://gist.github.com/chenkasirer/986edeb23f8e9ce6cf34fb0d4eb7ff9e

That's great!!!

@gonzalocasas
Copy link
Member

Lookin good @chenkasirer thanks for tag me! 🌠 just a tiny missing datatype for RV8. As mentioned by @gonzalocasas the action (and readme) should be updated to have an additional interpreter otpion where in the workflow there would be:

- uses: compas-dev/compas-actions.ghpython_components@v5
        with:
          source: components
          target: build
          interpreter: ipyV8  # or something like this

And the action.yml would have just another case:

- name: Launch componentizer
run: |
if ("${{ inputs.interpreter }}" -eq "cpython") {
$command="python"
$componentizer="${{ github.action_path }}/componentize_cpy.py"
} else {
$command="ipy"
$componentizer="${{ github.action_path }}/componentize_ipy.py"
}
$params=$componentizer, "${{ inputs.source }}", "${{ inputs.target }}", "--ghio", "./lib"
$prefix="${{ inputs.prefix }}"
if( $prefix )
{
$params=$params + "--prefix", "$prefix"
}
& $command $params
shell: pwsh

@9and3 thanks so much for taking a look!
I missed a bunch of guids that changes etc, but I think I got it right now.

What do we think about ipy_v2 instead of ipyV8? I tend to like ipy_v2 better..

Shouldn't we use the legacy keyword to identify the old stuff like rhino does? ie cpython and ironpython for the new editor and ironpython_legacy for the old one?

@9and3
Copy link
Contributor

9and3 commented Sep 14, 2024

Shouldn't we use the legacy keyword to identify the old stuff like rhino does? ie cpython and ironpython for the new editor and ironpython_legacy for the old one?

indeed!

@chenkasirer
Copy link
Member Author

Shouldn't we use the legacy keyword to identify the old stuff like rhino does? ie cpython and ironpython for the new editor and ironpython_legacy for the old one?

I like this suggestion.
What I'm worried about it that it would make IronPython2 components the default that are packaged with releases. These aren't backwards compatible. We need to come up with some mechanism that lets us package all versions with release and lets typical users a way to choose which ones to install (as Rhino7 and 8 share the same component directory, you can really have only one or another installed).

@chenkasirer
Copy link
Member Author

@gonzalocasas @9and3 so what do we say? I think we should leave the default behavior to be the legacy one to not break Rhino7 compatibility (this action is used directly from main..)
Once we have a good strategy of dealing with the different environments we can do this rework.

@gonzalocasas
Copy link
Member

Sorry, I forgot about this. There's been additional changes and decisions on this topic. What do we do with this PR? Is it still relevant?

@tomvanmele
Copy link
Member

i vote, move forward and don't look back :)

@chenkasirer
Copy link
Member Author

chenkasirer commented Jan 29, 2025

yeah this seemed like a good idea back then, but now I'm not sure why you'd want to do your GH plugin using ironpython, especially since that r syntax thingy doesn't really work there.

we could merge this as it is, and if someone really want they can make rhino8 ipy comonents, otherwise everything else stays the same. or we could also scrap this, my eyes are on cpython atm.

@gonzalocasas
Copy link
Member

Don't look back sounds great to me. Time to plan a COMPAS 3.x release entirely dropping IPY support across the board?

@chenkasirer
Copy link
Member Author

Don't look back sounds great to me. Time to plan a COMPAS 3.x release entirely dropping IPY support across the board?

amen

@gonzalocasas
Copy link
Member

we could merge this as it is

👍 go for it!

@tomvanmele
Copy link
Member

am keeping this for the COMPAS museum. just in case...

Screenshot 2025-01-29 at 13 52 08

@chenkasirer chenkasirer merged commit 3d6f04c into main Jan 29, 2025
3 checks passed
@chenkasirer chenkasirer deleted the ipy_v2 branch January 29, 2025 16:24
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