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: remove safe_import and all usages #5139

Merged
merged 10 commits into from
Jun 26, 2023
Merged

chore: remove safe_import and all usages #5139

merged 10 commits into from
Jun 26, 2023

Conversation

ZanSara
Copy link
Contributor

@ZanSara ZanSara commented Jun 13, 2023

Related Issues

Proposed Changes:

  • Removes safe_import and all its usages from the codebase

How did you test it?

  • CI

Notes for the reviewer

n/a

Checklist

@coveralls
Copy link
Collaborator

coveralls commented Jun 13, 2023

Pull Request Test Coverage Report for Build 5376570007

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 1076 unchanged lines in 25 files lost coverage.
  • Overall coverage increased (+0.6%) to 43.069%

Files with Coverage Reduction New Missed Lines %
nodes/file_classifier/file_type.py 1 98.28%
nodes/file_converter/markdown.py 1 98.25%
nodes/file_converter/init.py 2 88.24%
nodes/prompt/shapers.py 2 91.3%
utils/openai_utils.py 5 90.22%
agents/base.py 6 95.81%
errors.py 6 78.0%
modeling/data_handler/dataloader.py 8 81.4%
nodes/other/shaper.py 13 92.4%
document_stores/es_converter.py 16 21.57%
Totals Coverage Status
Change from base Build 5335925213: 0.6%
Covered Lines: 9759
Relevant Lines: 22659

💛 - Coveralls

@ZanSara ZanSara marked this pull request as ready for review June 22, 2023 09:03
@ZanSara ZanSara requested a review from a team as a code owner June 22, 2023 09:03
@ZanSara ZanSara requested review from bogdankostic and removed request for a team June 22, 2023 09:03
Copy link
Contributor

@bogdankostic bogdankostic left a comment

Choose a reason for hiding this comment

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

Almost good to go! 👍🏽 I have just one question about a case where I think the behavior changed.

haystack/document_stores/opensearch.py Outdated Show resolved Hide resolved
@@ -62,6 +56,7 @@ def _estimate_extension(self, file_path: Path) -> str:
:param file_path: the path to extract the extension from
"""
try:
magic_import.check()
Copy link
Contributor

Choose a reason for hiding this comment

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

Previously, we were just logging a debug message if magic could not have been imported, this will now probably raise an error - is this intended?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah good point... I added it to the try-catch block but then I didn't catch the proper error 😅 I'll fix line 62 to catch this one too!

@ZanSara ZanSara requested a review from bogdankostic June 26, 2023 09:38
Copy link
Contributor

@bogdankostic bogdankostic left a comment

Choose a reason for hiding this comment

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

🧹

@ZanSara ZanSara merged commit 7a9cf30 into main Jun 26, 2023
@ZanSara ZanSara deleted the remove-safe-import branch June 26, 2023 10:42
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.

Remove custom optional imports
3 participants