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

Refactored tools to not utilize manifest.yaml #1226

Merged
merged 4 commits into from
Oct 4, 2024

Conversation

william-price01
Copy link
Contributor

@william-price01 william-price01 commented Oct 4, 2024

Describe your changes

Removed manifest.yaml requirements.

Issue ticket number and link


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

@william-price01 william-price01 marked this pull request as ready for review October 4, 2024 17:40
@william-price01 william-price01 requested review from collindutter, cjkindel and dylanholmes and removed request for collindutter October 4, 2024 17:40
@cjkindel
Copy link
Contributor

cjkindel commented Oct 4, 2024

Do we have an Issue or other information on why this change is being made and what its impact is?

@william-price01
Copy link
Contributor Author

Do we have an Issue or other information on why this change is being made and what its impact is?

@collindutter requested I remove this from the framework.

@collindutter
Copy link
Member

@cjkindel We've got an internal issue for this. manifest.yaml is not used in any way. It's a file from when we had different plans for Tools.

@@ -4,24 +4,11 @@ Building your own tools is easy with Griptape!

To start, create a directory for your tool inside your project. All tool directories should have the following components:

* `manifest.yml` with a YAML manifest.
* `tool.py` file with a tool Python class.
* `requirements.txt` file with tool Python dependencies.
Copy link
Member

Choose a reason for hiding this comment

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

@william-price01 while we're already in here, can you test if this is true? I think this is optional I could be wrong.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Test if requirements.txt is required, if your tool must be called tool.py?
@collindutter

Copy link
Member

Choose a reason for hiding this comment

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

Actually, both would be good to test.

Copy link

codecov bot commented Oct 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

📢 Thoughts on this report? Let us know!

@vasinov
Copy link
Member

vasinov commented Oct 4, 2024

Let's hold off on this one as we might actually start using it for hosted tools.

@william-price01 william-price01 marked this pull request as draft October 4, 2024 18:10
@william-price01 william-price01 marked this pull request as ready for review October 4, 2024 18:56
@collindutter
Copy link
Member

@vasinov and I talked offline. We're going to move forward with this change. Any hosted tool manifest will be a file unique to that cloud feature, not a part of the framework.

@collindutter collindutter force-pushed the Refactor/Tool_creation branch from c6e7b1d to 31de3ea Compare October 4, 2024 22:08
@collindutter collindutter merged commit 661e5e3 into dev Oct 4, 2024
15 checks passed
@collindutter collindutter deleted the Refactor/Tool_creation branch October 4, 2024 22:22
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.

4 participants