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

chore: Cohere namespace change #247

Merged
merged 9 commits into from
Jan 22, 2024
Merged

chore: Cohere namespace change #247

merged 9 commits into from
Jan 22, 2024

Conversation

vblagoje
Copy link
Member

@vblagoje vblagoje commented Jan 19, 2024

Why:

The changes in this pull request are focused on restructuring and reorganizing the codebase for better modularity and clarity. The modifications include updates to project configuration files, renaming of files and directories, and updating references to match the new structure. These adjustments enhance the maintainability of the code and align with a more standardized project layout.

What:

  • Updated the pyproject.toml file to refer to new package locations and configuration settings.
  • Added new __init__.py files to define public interfaces for embedders and generators components, ensuring clean imports and namespace management.
  • Moved and renamed several embedders and generators modules to fall under a more descriptive haystack_integrations namespace.
  • Adjusted test files to reference the renamed modules, ensuring consistency and passing tests.

How can it be used:

  • The pyproject.toml updates can be used to accurately reflect dependencies and project configuration for building the wheel.
  • The new __init__.py files allow users to import classes like CohereDocumentEmbedder, CohereTextEmbedder, and CohereGenerator more intuitively using the updated paths.
  • The renamed modules help clarify their purpose and usage within the project, such as:
    from haystack_integrations.components.embedders import CohereDocumentEmbedder

How did you test it:

  • The updates in the test modules, such as test_cohere_chat_generator.py and test_document_embedder.py
  • Additional testing should include running the full suite of unit tests, as well as integration tests to confirm that the new structure doesn't break any existing functionality.

Notes for the reviewer:

  • Ensure that the new structure follows the Python package conventions and does not break any imports elsewhere in the project.
  • Verify that all tests, including integration tests not shown in the PR diff, pass after the restructuring.
  • Scrutinize the pyproject.toml changes to ensure that all necessary configurations are appropriately updated and no essential configurations are omitted.

@vblagoje vblagoje requested a review from a team as a code owner January 19, 2024 13:51
@vblagoje vblagoje requested review from silvanocerza and removed request for a team January 19, 2024 13:51
Copy link
Contributor

@masci masci left a comment

Choose a reason for hiding this comment

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

Overall looks good, one missing thing, we need an additional cohere folder so that the import becomes:
from haystack_integrations.components.embedders.cohere import CohereDocumentEmbedder

This is needed because otherwise different integrations might mount generators with the same name under haystack_integrations.components.embedders, overwriting each other

@masci masci removed the request for review from silvanocerza January 19, 2024 14:26
@vblagoje
Copy link
Member Author

🤦 have another look @masci

@masci
Copy link
Contributor

masci commented Jan 19, 2024

@vblagoje can you remove the .idea/ folder? :)

@vblagoje
Copy link
Member Author

O wow 🤣

Copy link
Contributor

@masci masci left a comment

Choose a reason for hiding this comment

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

LGTM Thanks!

@vblagoje vblagoje merged commit f8efcfe into main Jan 22, 2024
9 checks passed
@vblagoje vblagoje deleted the cohere_namespace_change branch January 22, 2024 09:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants