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: introduce Config class and update E2EPipelineFactory to use it #172

Merged
merged 2 commits into from
Mar 14, 2024

Conversation

emrgnt-cmplxty
Copy link
Contributor

@emrgnt-cmplxty emrgnt-cmplxty commented Mar 14, 2024

Ellipsis 🚀 This PR description was created by Ellipsis for commit 02cffa2.

Summary:

This PR introduces a new Config class for handling configuration data, updates the E2EPipelineFactory to use this class, adds type hints to several functions, and updates example files to use the new load_config function.

Key points:

  • Introduced a new Config class in r2r/main/utils.py for handling configuration data.
  • Updated load_config function to return an instance of Config.
  • Updated E2EPipelineFactory in r2r/main/factory.py to use Config.
  • Updated create_pipeline function to take a config argument of type Config.
  • Added type hints to get_vector_db, get_embeddings_provider, get_llm, and get_text_splitter functions.
  • Updated r2r/examples/academy/app.py, r2r/examples/basic/app.py, and r2r/examples/web_search/app.py to use load_config when creating a pipeline.
  • Minor modification in r2r/core/providers/logging.py.

Generated with ❤️ by ellipsis.dev

Copy link

vercel bot commented Mar 14, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
r2-r ✅ Ready (Inspect) Visit Preview 💬 Add feedback Mar 14, 2024 7:37am
r2r-cloud ❌ Failed (Inspect) Mar 14, 2024 7:37am
r2r-cloud-docs ❌ Failed (Inspect) Mar 14, 2024 7:37am

@emrgnt-cmplxty emrgnt-cmplxty changed the title add changes ... Mar 14, 2024
@emrgnt-cmplxty emrgnt-cmplxty marked this pull request as ready for review March 14, 2024 07:38
@ellipsis-dev ellipsis-dev bot changed the title ... feat: introduce Config class and update E2EPipelineFactory to use it Mar 14, 2024
Copy link
Contributor

@ellipsis-dev ellipsis-dev bot 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 to me!

  • Reviewed the entire pull request up to 1f94eaf
  • Looked at 257 lines of code in 6 files
  • Took 2 minutes and 9 seconds to review
More info
  • Skipped 2 files when reviewing.
  • Skipped posting 1 additional comments because they didn't meet confidence threshold of 50%.
1. r2r/main/factory.py:72:
  • Assessed confidence : 0%
  • Comment:
    The changes in this PR improve the readability and maintainability of the code by introducing a Config object to handle configuration data. The type hints for the get_vector_db, get_embeddings_provider, get_llm, and get_text_splitter functions have been updated accordingly. I didn't find any issues with these changes.
  • Reasoning:
    The PR author has refactored the configuration loading process. Instead of returning a tuple of configurations, the load_config function now returns a Config object. This object is then passed to the create_pipeline function of the E2EPipelineFactory class. This change makes the code more readable and maintainable. The PR author has also updated the type hints for the get_vector_db, get_embeddings_provider, get_llm, and get_text_splitter functions to reflect the new Config object. I didn't find any logical bugs, performance bugs, or security bugs in the changes. The PR author has followed best practices.

Workflow ID: wflow_UcXq0vDFwK94bpSo


Not what you expected? You can customize the content of the reviews using rules. Learn more here.

@emrgnt-cmplxty emrgnt-cmplxty changed the base branch from main to dev March 14, 2024 11:32
@emrgnt-cmplxty emrgnt-cmplxty merged commit 02cffa2 into dev Mar 14, 2024
2 of 4 checks passed
Copy link
Contributor

@ellipsis-dev ellipsis-dev bot 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 to me!

  • Performed an incremental review on 02cffa2
  • Looked at 256 lines of code in 6 files
  • Took 2 minutes and 3 seconds to review
More info
  • Skipped 1 files when reviewing.
  • Skipped posting 1 additional comments because they didn't meet confidence threshold of 50%.
1. r2r/main/utils.py:26:
  • Assessed confidence : 0%
  • Comment:
    The Config class is a good addition to the codebase. It provides a clear and structured way to handle configuration data. The changes in the E2EPipelineFactory class are also well done, with the new Config class being used effectively. The type hints added to the functions in this class are also a good addition, making the code more readable and easier to understand. The changes in the example files are also well done, with the new load_config function being used correctly. Overall, the changes in this PR are well done and I didn't find any issues or bugs.
  • Reasoning:
    The Config class is a good addition to the codebase. It provides a clear and structured way to handle configuration data. The changes in the E2EPipelineFactory class are also well done, with the new Config class being used effectively. The type hints added to the functions in this class are also a good addition, making the code more readable and easier to understand. The changes in the example files are also well done, with the new load_config function being used correctly. Overall, the changes in this PR are well done and I didn't find any issues or bugs.

Workflow ID: wflow_t0S2RqBjJtFkf9YS


Not what you expected? You can customize the content of the reviews using rules. Learn more here.

@emrgnt-cmplxty emrgnt-cmplxty deleted the feature/de-couple-config-factory branch March 14, 2024 21:22
emrgnt-cmplxty added a commit that referenced this pull request Mar 15, 2024
* feat: introduce Config class and update E2EPipelineFactory to use it (#172)

* add changes

* decouple config

* Feature/blast web and chat (#173)

* add changes

* decouple config

* blast web and chat

* Feature/prompt provider and type reno (#174)

* modify

* mostly complete.

* tweak types

* Feature/add app field to config (#175)

* modify

* mostly complete.

* tweak types

* fix config name issue, add app field

* tweak imports and all that

* Feature/add redis config option (#176)

* modify

* mostly complete.

* tweak types

* add redis config option

* erase cruft

* Feature/add redis config option (#177)

* modify

* mostly complete.

* tweak types

* add redis config option

* erase cruft

* update package deps (#178)
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.

None yet

1 participant