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

✨ Xircuits Context #109

Merged
merged 6 commits into from
Mar 1, 2022
Merged

✨ Xircuits Context #109

merged 6 commits into from
Mar 1, 2022

Conversation

MFA-X-AI
Copy link
Member

@MFA-X-AI MFA-X-AI commented Feb 25, 2022

Description

This pull request introduces xircuits context (ctx),which allows users to pass data to another components in the form of a python dict.
Previously to pass any type of data, the user needs to be explicitly code it via outPort to inPorts. This would be cumbersome especially if it's a global variable that might be used everywhere, such as the experiment name. By using ctx, users simply need to add it in their execute section. Here is a basic syntax:

@xai_component
class HelloContext(Component):

    def __init__(self, ctx):

    def execute(self, ctx) -> None:

        context_dict = {"new ctx": "Hello Xircuits!"}
        ctx.update(context_dict)

I have added the HelloContext component to the example components library for you to try.

Pull Request Type

  • Xircuits Core (Jupyterlab Related changes)
  • Xircuits Canvas (Custom RD Related changes)
  • Xircuits Component Library
  • Documentation
  • Others (Please Specify)

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. Ensure that ctx can be accessed by all components

    1. Drag 2 `Hello_Context" and link them together.
    2. The context should be accessible in the next component

You may also try out this xircuit.

Expected Output:

image

  1. Ensure that previous components are still working

    1. Run any Xircuits examples. Shouldn't get any errors.
    2. Check the github E2E github action. Should pass the test.

Tested on?

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

Notes

As the ctx parameter needs to be passed in, I had to update every component in this pull request. You will need to pull a fresh copy / not use your old component library.

@MFA-X-AI MFA-X-AI marked this pull request as ready for review February 25, 2022 08:46
@MFA-X-AI MFA-X-AI requested a review from AdrySky February 25, 2022 08:47
@AdrySky
Copy link
Contributor

AdrySky commented Feb 28, 2022

I got an error running xircuits's example.
Error

Also, I'm not sure is this how the context should work. I thought we pass the data across the component internally, not just only to next node.

For example, let say I make a xircuits below
example

Assuming, the input_str for HelloHyperparameter is empty. I want to use the comp_str from CompulsoryHyperparameter as the input_str for HelloHyperparameter.

So the expected result when it's executing the HelloHyperparameter should be 'Hello, context_test', not 'Hello, test' or 'Hello, {default_value}'.

I might be wrong but I assumed that how the context should work.

@MFA-X-AI
Copy link
Member Author

MFA-X-AI commented Mar 1, 2022

Thanks for the review!
I made a blunder in the usage for the ctx, should've had it in the execute() call. It should work now.

Assuming, the input_str for HelloHyperparameter is empty. I want to use the comp_str from CompulsoryHyperparameter as the input_str for HelloHyperparameter.
So the expected result when it's executing the HelloHyperparameter should be 'Hello, context_test', not 'Hello, test' or 'Hello, {default_value}'.
I might be wrong but I assumed that how the context should work.

Really good question, thanks for asking!
You're on track but the user needs to be more verbose in the implementation. In your use case the user would need to update the CompulsoryHyperparameter execution to:

    def execute(self, ctx) -> None:
        input_str = self.input_str.value
        comp_str = self.comp_str.value
        comp_int = self.comp_int.value
        print("Hello, " + str(input_str))
        print("I'm " + str(comp_str))
        print("Me " + str(comp_int))

        #save comp_str inside context as a dict
        ctx.update({"comp_str": comp_str}) 

And HelloHyperparameter to:

    def execute(self, ctx) -> None:
        # specify if the user does not supply input_str, use the context
        input_str = self.input_str.value if self.input_str.value else ctx['comp_str'] comp_str
        print("Hello, " + str(input_str))
        self.done = True

Here's the modified components gist:

Run that and you should get the following:
image

Copy link
Contributor

@AdrySky AdrySky left a comment

Choose a reason for hiding this comment

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

Tested and it's working as expected. Thanks for answering my question.

input_str = self.input_str.value if self.input_str.value else ctx['comp_str'] comp_str

Though, I think there a typo with the last comp_str. Once I remove that, it's working fine.

I just realized that this context can create a huge list of dict where any component can access that dict. Which is great..I think. From what I understood, I thought to access that context, we've to specify which component is the context is from like CompulsoryHyperparameter::comp_str (from what I remember how Mr. Eduardo did it).

Anyway, I'll merge this first.

@AdrySky AdrySky merged commit fc5850c into master Mar 1, 2022
@AdrySky AdrySky deleted the fahreza/xircuits-ctx branch March 1, 2022 04:53
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.

2 participants