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

feat(core): Introducing uagents-core #597

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

feat(core): Introducing uagents-core #597

wants to merge 6 commits into from

Conversation

qati
Copy link
Contributor

@qati qati commented Dec 23, 2024

Proposed Changes

[describe the changes here...]

Linked Issues

[if applicable, add links to issues resolved by this PR]

Types of changes

What type of change does this pull request make (put an x in the boxes that apply)?

  • Bug fix (non-breaking change that fixes an issue).
  • New feature added (non-breaking change that adds functionality).
  • Breaking change (fix or feature that would cause existing functionality to stop working as expected).
  • Documentation update.
  • Something else (e.g., tests, scripts, example, deployment, infrastructure).

Checklist

Put an x in the boxes that apply:

  • I have read the CONTRIBUTING guide
  • Checks and tests pass locally

If applicable

  • I have added tests that prove my fix is effective or that my feature works
  • I have added/updated the documentation (executed the script in python/scripts/generate_api_docs.py)

Further comments

[if this is a relatively large or complex change, kick off a discussion by explaining why you chose the solution you did, what alternatives you considered, etc...]

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess this is only used in utils, so maybe should be moved there?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wanted to introduce unified logging and log style to be adopted in the future.



class VerifiableModel(BaseModel):
agent_address: str
Copy link
Contributor

Choose a reason for hiding this comment

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

This will likely need an update very soon based on this PR: #603.

Copy link
Contributor

@jrriehl jrriehl left a comment

Choose a reason for hiding this comment

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

Looks good overall. My main comments are about the utilities. Also this should probably go under python/libs, right?

@qati qati marked this pull request as ready for review January 8, 2025 19:59
@qati qati requested review from Archento and lrahmani as code owners January 8, 2025 19:59
@qati qati requested a review from jrriehl January 9, 2025 10:25
Copy link
Member

@Archento Archento left a comment

Choose a reason for hiding this comment

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

Since there is a lot of copied/moved code it is actually quite hard to identify what you've changed or added. Also there is no description added to the PR so I can't really understand the reasoning for adding this library.

What is the main benefit for this structure compared to importing what you need for a project directly from uagents?



[tool.poetry.group.dev.dependencies]
black = "^24.10.0"
Copy link
Member

Choose a reason for hiding this comment

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

black is not used in this project and the lib right now does not have any tests so please remove all unneeded packages if this is meant to have its own pyproject file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

introducing to be used

@@ -1,5 +1,5 @@
# The default code owners of the uagents repo.
* @jrriehl @Archento @lrahmani
* @jrriehl @Archento @lrahmani @qati
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* @jrriehl @Archento @lrahmani @qati
* @jrriehl @Archento @lrahmani
/libs/core/ @qati

I propose this scope as your PR only targets changes in this directory.

libs/core/README.md Outdated Show resolved Hide resolved
libs/core/pyproject.toml Outdated Show resolved Hide resolved
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.

3 participants