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

✨Allow multiple link for port with string, list and dict type #137

Merged
merged 10 commits into from
Apr 8, 2022

Conversation

AdrySky
Copy link
Contributor

@AdrySky AdrySky commented Mar 31, 2022

Description

This enable connecting multiple link into port with string, list and dict type which in the codegen side, append the extra values.

Pull Request Type

  • Xircuits Core (Jupyterlab Related changes)
  • Xircuits Canvas (Custom RD Related changes)
  • Xircuits Component Library
  • Testing Automation
  • Documentation
  • Others (Code generator script)

Type of Change

  • New feature (non-breaking change which adds functionality)
  • Bug fix (non-breaking change which fixes an issue)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Tests

  1. Change any input port into string type.
    1. Try connecting 2 literal string.
    2. Print output.
    3. Repeat step(i) with literal list and dict.

Tested on?

  • Windows
  • Linux Ubuntu
  • Centos
  • Mac
  • Others (State here -> xxx )

Notes

The original PR focused on enabling multiports for the any type, but as discussed, the intended use case implementation via multiports could not be implemented in this PR.

@AdrySky AdrySky added the enhancement New feature or request label Mar 31, 2022
@AdrySky AdrySky requested a review from MFA-X-AI March 31, 2022 04:47
@AdrySky AdrySky self-assigned this Mar 31, 2022
Copy link
Member

@MFA-X-AI MFA-X-AI left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!
I'm getting some issues in the codegen side of things, literal nodes are sometimes returning undefined values. Here's the gist for KerasTrainImageClassifier.
https://gist.github.com/MFA-X-AI/9be15e0abafb7003371905adb3c7ed28
To reproduce, simply run the codeegn.

I'm investigating further what causes it.

@AdrySky
Copy link
Contributor Author

AdrySky commented Apr 5, 2022

Sorry. I forgot to put value for default type.

Also, I allow multiple links for same link and port type. Not just for any.

@AdrySky AdrySky requested a review from MFA-X-AI April 5, 2022 07:24
@mansouralawi
Copy link
Member

mansouralawi commented Apr 6, 2022

Thanks for the fixes.

  • Literal nodes are sometimes returning undefined values: issue is fixed
  • Append list,dict,str when input node is [any] type working fine

issue

1- Append list,dict,str when the input node is [list,dict or str] type not working
2- Appending literals although they belong to different input nodes. to reproduce try AutoMLClassificationBlendModels.xircuits

multi_error

@AdrySky
Copy link
Contributor Author

AdrySky commented Apr 6, 2022

Thanks @mansouralawi for the review. Both issue fixed on my side. Appreciate if you could review again.

@AdrySky AdrySky requested a review from mansouralawi April 6, 2022 08:06
Copy link
Member

@mansouralawi mansouralawi left a comment

Choose a reason for hiding this comment

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

Thanks for the fixes.

  • Literal nodes are sometimes returning undefined values: issue is fixed
  • Append list,dict,str when input node is [any] type working fine
  • Append list,dict,str when the input node is [list,dict or str] type working fine
  • Appending literals although they belong to different input nodes. 'issue is fixed'

Issue

1- Appending 2 trained models of type [any] to input node type [any] produce the error below. (The target is to append the two models to a list)

image
outarg_error

@MFA-X-AI MFA-X-AI changed the title ✨Allow multiple link for port with 'any' type ✨Allow multiple link for port with string, list and dict type Apr 8, 2022
Copy link
Member

@MFA-X-AI MFA-X-AI left a comment

Choose a reason for hiding this comment

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

As discussed, the any implementation requires additional features to be added, but for string list and dict it's perfect. Tested and working.
One interesting to note was the order of the append - it follows the order of when the link was created. In the future it would be cool to indicate the order, perhaps by link label.

@MFA-X-AI MFA-X-AI merged commit 140ab68 into master Apr 8, 2022
@MFA-X-AI MFA-X-AI deleted the adry/allow-multiple-link branch April 8, 2022 03:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants