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

fix component as a tool #4166

Closed
wants to merge 12 commits into from
Closed

Conversation

edwinjosechittilappilly
Copy link
Collaborator

@edwinjosechittilappilly edwinjosechittilappilly commented Oct 15, 2024

added class variable is_tool that enable the component to be tool. added basic bool input to turn on/off as tool.
TODO to solve But the toggle doesnt update is_tool as true or changes the output.

@edwinjosechittilappilly edwinjosechittilappilly marked this pull request as ready for review October 16, 2024 15:15
@dosubot dosubot bot added the size:M This PR changes 30-99 lines, ignoring generated files. label Oct 16, 2024
src/backend/base/langflow/inputs/inputs.py Outdated Show resolved Hide resolved
self._previous_value = self.value
return self

def __deepcopy__(self, memo):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you explain why this was necessary?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

deepcopy was creating issues due to recursion.
Hence overriding that to prevent that issues

Copy link
Collaborator

Choose a reason for hiding this comment

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

This seems potentially very risky, regarding mutable attributes. You're only copying mutable attributes from the BoolInput. However, BaseInputMixin (and the others) also contain mutable attributes.

Future devs that add mutable attributes to these mixins won't know that they need to also update BoolInput, else they risk sharing state between instances.

Let's sync up later and see if there are any alternatives.

Copy link
Contributor

Choose a reason for hiding this comment

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

I added a fix for deepcopy (it was missing passing memo to the inner deepcopy calls). It is in main already.

@@ -71,7 +72,9 @@ def ensure_url(self, string: str) -> str:
return string

def fetch_content(self) -> list[Data]:
urls = [self.ensure_url(url.strip()) for url in self.urls if url.strip()]
# check if the urls are list or not
urls_list = [self.urls] if isinstance(self.urls, str) else self.urls
Copy link
Collaborator

Choose a reason for hiding this comment

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

If self.urls is None, does the below behavior still work? Or do we need to add an explicit None check and return an error?

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with Jordan. I'd say a check could be useful to catch unexpected errors. This should never happen but will help us avoid regression.

@@ -17,6 +17,7 @@
from langflow.helpers.custom import format_type
from langflow.schema.artifact import get_artifact_type, post_process_raw
from langflow.schema.data import Data
from langflow.schema.dotdict import dotdict # noqa: TCH001
Copy link
Collaborator

Choose a reason for hiding this comment

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

Did you run into an issue putting this in a TYPE_CHECKING block?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Moved to typechecking block

self._previous_value = self.value
return self

def __deepcopy__(self, memo):
Copy link
Collaborator

Choose a reason for hiding this comment

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

This seems potentially very risky, regarding mutable attributes. You're only copying mutable attributes from the BoolInput. However, BaseInputMixin (and the others) also contain mutable attributes.

Future devs that add mutable attributes to these mixins won't know that they need to also update BoolInput, else they risk sharing state between instances.

Let's sync up later and see if there are any alternatives.

self._previous_value = self.value
return self

def __deepcopy__(self, memo):
Copy link
Contributor

Choose a reason for hiding this comment

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

I added a fix for deepcopy (it was missing passing memo to the inner deepcopy calls). It is in main already.

@@ -71,7 +72,9 @@ def ensure_url(self, string: str) -> str:
return string

def fetch_content(self) -> list[Data]:
urls = [self.ensure_url(url.strip()) for url in self.urls if url.strip()]
# check if the urls are list or not
urls_list = [self.urls] if isinstance(self.urls, str) else self.urls
Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with Jordan. I'd say a check could be useful to catch unexpected errors. This should never happen but will help us avoid regression.

@dosubot dosubot bot added size:S This PR changes 10-29 lines, ignoring generated files. and removed size:M This PR changes 30-99 lines, ignoring generated files. labels Oct 22, 2024
added class variable is_tool that enable the component to be tool.
added basic bool input to turn on/off as tool.
TOdO to solve But the toggle doesnt update is_tool as true or changes the output.
…efinition

- Changed the definition of 'value' to use Field for default value assignment.
- Updated 'onChange' to use Field with exclude and repr options.
- Ensured that the model configuration allows for assignment validation and arbitrary types.
- Added a model validator to check for value changes and trigger the onChange callback.
Tool calling agent may provide a URL component with text data instead of list data. Hence, this is being fixed.
shall update once the Dynamic function is available and utilise the build config to make the update.
@dosubot dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. and removed size:S This PR changes 10-29 lines, ignoring generated files. labels Oct 23, 2024
moved todict to type checking block
@edwinjosechittilappilly edwinjosechittilappilly marked this pull request as draft October 23, 2024 19:02
@edwinjosechittilappilly
Copy link
Collaborator Author

This PR is Set to draft:
Multiple PR created solving individual issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size:L This PR changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants