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

feat(handler): add index field to support chained handlers #343

Merged

Conversation

mostaphaRoudsari
Copy link
Member

One of the creative usage of handlers that came up during the development was to use handlers one after another. Luckily the current schema could support this case:

    OutputAlias.linked(
        name='results',
        platform=['rhino'],
        handler=[
            # Preload results 
            IOAliasHandler(
                language='python',
                module='pollination_handlers.outputs.daylight',
                function='read_df_from_folder'
            ),
            # load preloaded outputs to Rhino with following method
            IOAliasHandler(
                language='csharp', module='Pollination.RhinoHandlers',
                function='LoadMeshBasedResultsToRhino'
            )
        ]
    )

The current implementation doesn't indicate which handler should be executed first. In this case since @MingboPeng was the developer of both the Rhino plugin and the Alias he knew how to get it to work but it wouldn't work otherwise.

This commit adds an optional index field to IOALiasHandler object to support supporting chained handlers. Default value will be set to 0. That means the code above will look like this:

    OutputAlias.linked(
        name='results',
        platform=['rhino'],
        handler=[
            # Preload results 
            IOAliasHandler(
                language='python',
                module='pollination_handlers.outputs.daylight',
                function='read_df_from_folder', index=0
            ),
            # load preloaded outputs to Rhino with following method
            IOAliasHandler(
                language='csharp', module='Pollination.RhinoHandlers',
                function='LoadMeshBasedResultsToRhino', index=1
            )
        ]
    )

This change will make it explicit that the Rhino platform should use the first handler for reading the results and execute that in Python - then pass the results to the second handler and run that one in C# which is the intended behavior.

One of the creative usage of handlers that [came up](https://github.com/pollination/pollination-alias/blob/21a0db5569c739cfb4f0de6a1dc8b3f1721bfd4c/pollination/alias/outputs/daylight.py#L25-L38) during the development was to use handlers one after another. Luckily the current schema could support this case:

```python
    OutputAlias.linked(
        name='results',
        platform=['rhino'],
        handler=[
            # Preload results 
            IOAliasHandler(
                language='python',
                module='pollination_handlers.outputs.daylight',
                function='read_df_from_folder'
            ),
            # load preloaded outputs to Rhino with following method
            IOAliasHandler(
                language='csharp', module='Pollination.RhinoHandlers',
                function='LoadMeshBasedResultsToRhino'
            )
        ]
    )
```

The current implementation doesn't indicate which handler should be executed first. In this case since @MingboPeng was the developer of both the Rhino plugin and the Alias he knew how to get it to work but it wouldn't work otherwise.

This commit adds an optional index field to IOALiasHandler object to support supporting chained handlers. Default value will be set to 0. That means the code above will look like this:

```python

    OutputAlias.linked(
        name='results',
        platform=['rhino'],
        handler=[
            # Preload results 
            IOAliasHandler(
                language='python',
                module='pollination_handlers.outputs.daylight',
                function='read_df_from_folder', index=0
            ),
            # load preloaded outputs to Rhino with following method
            IOAliasHandler(
                language='csharp', module='Pollination.RhinoHandlers',
                function='LoadMeshBasedResultsToRhino', index=1
            )
        ]
    )
```

This change will make it explicit that the Rhino platform should use the first handler for reading the results and execute that in Python - then pass the results to the second handler and run that one in C# which is the intended behavior.
@mostaphaRoudsari mostaphaRoudsari added the enhancement New feature or request label May 26, 2021
@mostaphaRoudsari mostaphaRoudsari self-assigned this May 26, 2021
@coveralls
Copy link

coveralls commented May 26, 2021

Pull Request Test Coverage Report for Build 880089404

  • 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.02%) to 49.007%

Totals Coverage Status
Change from base Build 879471130: 0.02%
Covered Lines: 1407
Relevant Lines: 2871

💛 - Coveralls

@mostaphaRoudsari mostaphaRoudsari merged commit 325f6be into pollination:master May 26, 2021
@ladybugbot
Copy link

🎉 This PR is included in version 1.25.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@mostaphaRoudsari mostaphaRoudsari deleted the chained-handlers branch May 27, 2021 01:42
Copy link
Member

@AntoineDao AntoineDao left a comment

Choose a reason for hiding this comment

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

Hi @mostaphaRoudsari, can we slightly change this implementation to make it safer for the backend to integrate?

@@ -410,3 +410,12 @@ class IOAliasHandler(BaseModel):
description='Name of the function. The input value will be passed to this '
'function as the first argument.'
)

index: int = Field(
0,
Copy link
Member

Choose a reason for hiding this comment

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

Any chance we can replace this a nullable field rather than a default one? This will make the recipe digest more reliable when comparing with digests already present on the database.

index: Optional[int] = Field(None, description='An integer...'

Copy link
Member Author

Choose a reason for hiding this comment

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

Hi @AntoineDao, Thanks for the note. I should have waited for your before merging this in. I remember we had issues with Optional when we used it in Honeybee Schema: ladybug-tools/honeybee-schema#121 (comment) - optional doesn't really translate to OpenAPI and that can cause inconsistencies.

I know that this might generate a new hash for the current packages the first time but it will be fine after that.

Copy link
Member

Choose a reason for hiding this comment

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

I think waiting would have been helpful here. I'm a bit nervous about what will happen when we merge this into pollination-server so I will push it now and we can keep an eye on it to see if it breaks anything

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds good! Based on what we talked about before - adding new optional fields should be fine but you never know. 🤞

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants