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

fix provider defaults issue #1667

Merged
merged 1 commit into from
Dec 6, 2024
Merged

Conversation

emrgnt-cmplxty
Copy link
Contributor

@emrgnt-cmplxty emrgnt-cmplxty commented Dec 6, 2024

Important

Improved IngestionConfig default handling with dynamic methods and added logging for effective configuration in documents_router.py.

  • IngestionConfig Defaults:
    • Introduced _defaults class variable in IngestionConfig to store default values.
    • Added set_default() and get_default() methods to dynamically manage default values.
    • Updated fields in IngestionConfig to use default_factory for defaults.
  • Logging:
    • Added a debug print statement in create_document() in documents_router.py to log effective_ingestion_config.
  • Version Update:
    • Incremented version in pyproject.toml from 3.3.3 to 3.3.4.

This description was created by Ellipsis for fa302bf. It will automatically update as commits are pushed.

@emrgnt-cmplxty emrgnt-cmplxty marked this pull request as ready for review December 6, 2024 18:28
@emrgnt-cmplxty emrgnt-cmplxty merged commit 9735323 into main Dec 6, 2024
1 of 19 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.

❌ Changes requested. Reviewed everything up to fa302bf in 1 minute and 24 seconds

More details
  • Looked at 210 lines of code in 4 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. py/core/main/api/v3/documents_router.py:278
  • Draft comment:
    Remove the print statement used for debugging purposes to clean up the production code.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The print statement is likely used for debugging and should be removed in production code.

Workflow ID: wflow_xxH0nV9bA1hN6gPN


Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

return cls(app=app, parser_overrides={"pdf": "zerox"})
else:
return cls(app=app)

@classmethod
def get_default(cls, mode: str, app) -> "IngestionConfig":
Copy link
Contributor

Choose a reason for hiding this comment

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

The get_default method is defined twice in the IngestionConfig class. Consider removing one of the definitions to avoid redundancy and potential confusion.

@@ -59,6 +122,36 @@
return cls(app=app)


@classmethod
def set_default(cls, **kwargs):
Copy link
Contributor

Choose a reason for hiding this comment

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

The set_default method is defined twice in the IngestionConfig class. Consider removing one of the definitions to avoid redundancy and potential confusion.

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.

1 participant