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.

Sign up for GitHub

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

Jump to bottom

Standardize callables #1275

Merged
merged 1 commit into from
Oct 29, 2024
Merged

Standardize callables #1275

merged 1 commit into from
Oct 29, 2024

Conversation

collindutter
Copy link
Member

@collindutter collindutter commented Oct 21, 2024 •
edited
Loading

Describe your changes

Relevant thread

Changed

  • BREAKING: Renamed callables throughout the framework for consistency:
    • Renamed LocalStructureRunDriver.structure_factory_fn to LocalStructureRunDriver.create_structure.
    • Renamed SnowflakeSqlDriver.connection_func to SnowflakeSqlDriver.get_connection.
    • Renamed CsvLoader.formatter_fn to CsvLoader.format_row.
    • Renamed SqlLoader.formatter_fn to SqlLoader.format_row.
    • Renamed CsvExtractionEngine.system_template_generator to CsvExtractionEngine.generate_system_template.
    • Renamed CsvExtractionEngine.user_template_generator to CsvExtractionEngine.generate_user_template.
    • Renamed JsonExtractionEngine.system_template_generator to JsonExtractionEngine.generate_system_template.
    • Renamed JsonExtractionEngine.user_template_generator to JsonExtractionEngine.generate_user_template.
    • Renamed PromptResponseRagModule.generate_system_template to PromptResponseRagModule.generate_system_template.
    • Renamed PromptTask.generate_system_template to PromptTask.generate_system_template.
    • Renamed ToolkitTask.generate_assistant_subtask_template to ToolkitTask.generate_assistant_subtask_template.
    • Renamed JsonSchemaRule.template_generator to JsonSchemaRule.generate_template.
    • Renamed ToolkitTask.generate_user_subtask_template to ToolkitTask.generate_user_subtask_template.
    • Renamed TextLoaderRetrievalRagModule.process_query_output_fn to TextLoaderRetrievalRagModule.process_query_output.
    • Renamed FuturesExecutorMixin.futures_executor_fn to FuturesExecutorMixin.create_futures_executor.
    • Renamed VectorStoreTool.process_query_output_fn to VectorStoreTool.process_query_output.
    • Renamed CodeExecutionTask.run_fn to CodeExecutionTask.on_run.
    • Renamed Chat.input_fn to Chat.handle_input.
    • Renamed Chat.output_fn to Chat.handle_output.
    • Renamed EventListener.handler to EventListener.on_event.

Issue ticket number and link

Closes #285

📚 Documentation preview 📚: https://griptape--1275.org.readthedocs.build//1275/

@codecov Codecov
Copy link

codecov bot commented Oct 21, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 97.10145% with 2 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
...rivers/structure_run/local_structure_run_driver.py 75.00% 0 Missing and 1 partial ⚠️
...iptape/engines/extraction/csv_extraction_engine.py 85.71% 1 Missing ⚠️

📢 Thoughts on this report? Let us know!

@collindutter collindutter mentioned this pull request Oct 21, 2024
Closed
@collindutter collindutter force-pushed the refactor/callable branch 2 times, most recently from 673d0f4 to 2c16ced Compare October 21, 2024 23:30
@collindutter collindutter marked this pull request as ready for review October 21, 2024 23:49
@vachillo
Copy link
Member

what is the reasoning between factory and provider?

@collindutter
Copy link
Member Author

Reposted my comment linked in the description.

*_factory: Used for functions that create a new instance of an object.
*_provider: Used for functions that supply a value. These functions may create new instances, as long as the value represents the same resource or value.

Multiple calls to a factory should give new objects with new meaning each time. For instance, 3 calls to structure_factory should give me 3 structures that I should treat as independent. 3 calls to user_template_provider may give me 3 J2 objects, but they represent the same thing under the hood.

dylanholmes
dylanholmes previously approved these changes Oct 23, 2024
View reviewed changes
Copy link
Contributor

@dylanholmes dylanholmes left a comment

Choose a reason for hiding this comment

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

Nice work! Like the renames.

Just optional suggestions. Don't necessarily need to be done in this PR either.

griptape/drivers/sql/snowflake_sql_driver.py Outdated
@@ -15,24 +15,26 @@

@define
class SnowflakeSqlDriver(BaseSqlDriver):
connection_func: Callable[[], SnowflakeConnection] = field(kw_only=True)
connection_provider: Callable[[], SnowflakeConnection] = field(kw_only=True)
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be nice to create some reusable types for these to make it easier for customer code create their own abstractions.

For example adding a:

ConnectionProvider = Callable[[], SnowflakeConnection]

In this file, then using it here:

connection_provider: ConnectionProvider = field(kw_only=True)

And you can reduce duplicate in def validate_connection_provider( too.

I can also see arguments against this, but I like being able to easily references types in my own code.

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah I like the idea, but that can be introduced in the future.

griptape/drivers/vector/local_vector_store_driver.py Outdated
relatedness_fn: Callable = field(default=lambda x, y: dot(x, y) / (norm(x) * norm(y)))
relatedness_calculator: Callable = field(default=lambda x, y: dot(x, y) / (norm(x) * norm(y)))
Copy link
Contributor

Choose a reason for hiding this comment

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

Why no generic params?

Copy link
Member Author

Choose a reason for hiding this comment

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

They should be there but I want to keep this PR strictly on naming.

tests/integration/tasks/test_text_summary_task.py
@@ -7,7 +7,7 @@ class TestTextSummaryTask:
@pytest.fixture(
autouse=True,
params=StructureTester.TEXT_SUMMARY_TASK_CAPABLE_PROMPT_DRIVERS,
ids=StructureTester.prompt_driver_id_fn,
ids=StructureTester.generate_prompt_driver_id,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why isn't this prompt_driver_id_provider or prompt_driver_id_factory?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because that's actually a class method, not a field. Methods should still be verb-noun.

vachillo
vachillo previously approved these changes Oct 23, 2024
View reviewed changes
@collindutter collindutter force-pushed the refactor/callable branch 5 times, most recently from c2170a0 to c0d713f Compare October 25, 2024 16:00
@collindutter collindutter requested a review from vachillo October 25, 2024 16:00
@collindutter collindutter force-pushed the refactor/callable branch 3 times, most recently from e3ab6d2 to eae2222 Compare October 29, 2024 15:40
vachillo
vachillo approved these changes Oct 29, 2024
View reviewed changes
Copy link
Member

@vachillo vachillo left a comment

Choose a reason for hiding this comment

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

lets update the PR description for posterity

@collindutter collindutter merged commit 3020a59 into dev Oct 29, 2024
15 checks passed
@collindutter collindutter deleted the refactor/callable branch October 29, 2024 16:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vachillo vachillo vachillo approved these changes

@SavagePencil SavagePencil Awaiting requested review from SavagePencil

@vasinov vasinov Awaiting requested review from vasinov

@dylanholmes dylanholmes Awaiting requested review from dylanholmes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Normalize Callable naming convention
3 participants
@collindutter @vachillo @dylanholmes