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: avoid exporting components to the components package #161

Merged
merged 1 commit into from
Jan 3, 2025

Conversation

davidsbatista
Copy link
Contributor

@davidsbatista davidsbatista commented Jan 2, 2025

Proposed Changes:

Make the imports in test as specific as possible, to avoid importing/testing other components when locally testing a single component.

How did you test it?

  • local unit tests,
  • integration tests, manual verification
  • CI tests

Checklist

@davidsbatista davidsbatista changed the title initial import fix: make components import in test as specific as possible Jan 2, 2025
@coveralls
Copy link

coveralls commented Jan 2, 2025

Pull Request Test Coverage Report for Build 12586724345

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.07%) to 83.128%

Totals Coverage Status
Change from base Build 12575655783: -0.07%
Covered Lines: 2025
Relevant Lines: 2436

💛 - Coveralls

@davidsbatista davidsbatista marked this pull request as ready for review January 2, 2025 18:06
@davidsbatista davidsbatista requested a review from a team as a code owner January 2, 2025 18:06
@davidsbatista davidsbatista requested review from vblagoje and anakin87 and removed request for a team January 2, 2025 18:06
Copy link
Member

@anakin87 anakin87 left a comment

Choose a reason for hiding this comment

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

I have nothing against this PR, but it seems to do something different from what is described.

Currently, this works:
from haystack_experimental.components import LLMMetadataExtractor

After this PR, the above import won't work and you will need to do
from haystack_experimental.components.extractors import LLMMetadataExtractor

It would be helpful if you could explain the motivation behind this choice. (It does not seem related to tests).

@davidsbatista
Copy link
Contributor Author

davidsbatista commented Jan 3, 2025

After this PR, the above import won't work and you will need to do
from haystack_experimental.components.extractors import LLMMetadataExtractor

check the file test/components/extractors/test_llm_metadata_extractor.py on the diff

@davidsbatista
Copy link
Contributor Author

davidsbatista commented Jan 3, 2025

When I want to run the tests for a single local file, e..g:

13:25 $ coverage run --source haystack_experimental/components/retrievers/ -m pytest -vvv -s test/components/retrievers/test_auto_merging_retriever.py && coverage report --show-missing

This is what happens, due to the the way the import in the test file is done

==================================================================================================== test session starts ====================================================================================================
platform darwin -- Python 3.12.6, pytest-8.3.3, pluggy-1.5.0 -- /Users/dsbatista/haystack-experimental/.venv/bin/python3
cachedir: .pytest_cache
rootdir: /Users/dsbatista/haystack-experimental
configfile: pyproject.toml
plugins: cov-6.0.0, anyio-4.6.0, rerunfailures-14.0
collected 0 items / 1 error

========================================================================================================== ERRORS ===========================================================================================================
________________________________________________________________________ ERROR collecting test/components/retrievers/test_auto_merging_retriever.py _________________________________________________________________________
test/components/retrievers/test_auto_merging_retriever.py:5: in <module>
    from haystack_experimental.components.splitters import HierarchicalDocumentSplitter
haystack_experimental/components/__init__.py:5: in <module>
    from .extractors import LLMMetadataExtractor
haystack_experimental/components/extractors/__init__.py:5: in <module>
    from haystack_experimental.components.extractors.llm_metadata_extractor import LLMMetadataExtractor, LLMProvider
haystack_experimental/components/extractors/llm_metadata_extractor.py:26: in <module>
    from haystack_integrations.components.generators.google_vertex import VertexAIGeminiGenerator
.venv/lib/python3.12/site-packages/haystack_integrations/components/generators/google_vertex/__init__.py:4: in <module>
    from .captioner import VertexAIImageCaptioner
.venv/lib/python3.12/site-packages/haystack_integrations/components/generators/google_vertex/captioner.py:4: in <module>
    import vertexai
.venv/lib/python3.12/site-packages/vertexai/__init__.py:19: in <module>
    from google.cloud.aiplatform import version as aiplatform_version
.venv/lib/python3.12/site-packages/google/cloud/aiplatform/__init__.py:24: in <module>
    from google.cloud.aiplatform import initializer
.venv/lib/python3.12/site-packages/google/cloud/aiplatform/initializer.py:38: in <module>
    from google.cloud.aiplatform.metadata import metadata
.venv/lib/python3.12/site-packages/google/cloud/aiplatform/metadata/metadata.py:29: in <module>
    from google.cloud.aiplatform import pipeline_jobs
.venv/lib/python3.12/site-packages/google/cloud/aiplatform/pipeline_jobs.py:33: in <module>
    from google.cloud.aiplatform.metadata import artifact
.venv/lib/python3.12/site-packages/google/cloud/aiplatform/metadata/artifact.py:26: in <module>
    from google.cloud.aiplatform import models
.venv/lib/python3.12/site-packages/google/cloud/aiplatform/models.py:49: in <module>
    from google.cloud.aiplatform import jobs
.venv/lib/python3.12/site-packages/google/cloud/aiplatform/jobs.py:1736: in <module>
    class CustomJob(_RunnableJob, base.PreviewMixin):
.venv/lib/python3.12/site-packages/google/cloud/aiplatform/jobs.py:2237: in CustomJob
    scheduling_strategy: Optional[gca_custom_job_compat.Scheduling.DiversityRankingStrategy] = None,
E   AttributeError: type object 'Scheduling' has no attribute 'DiversityRankingStrategy'
===================================================================================================== warnings summary ======================================================================================================
.venv/lib/python3.12/site-packages/_pytest/config/__init__.py:1441
  /Users/dsbatista/haystack-experimental/.venv/lib/python3.12/site-packages/_pytest/config/__init__.py:1441: PytestConfigWarning: Unknown config option: asyncio_mode

    self._warn_or_fail_if_strict(f"Unknown config option: {key}\n")

-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html
================================================================================================== short test summary info ==================================================================================================
ERROR test/components/retrievers/test_auto_merging_retriever.py - AttributeError: type object 'Scheduling' has no attribute 'DiversityRankingStrategy'
!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!! Interrupted: 1 error during collection !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
================================================================================================ 1 warning, 1 error in 4.37s ================================================================================================
/Users/dsbatista/haystack-experimental/.venv/lib/python3.12/site-packages/coverage/control.py:892: CoverageWarning: No data was collected. (no-data-collected)
  self._warn("No data was collected.", slug="no-data-collected")

I just want to test a single file and check it's coverage to understand where to focus on the tests, and due to the import it triggers unrelated tests

@davidsbatista davidsbatista requested a review from anakin87 January 3, 2025 12:32
Copy link
Member

@anakin87 anakin87 left a comment

Choose a reason for hiding this comment

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

Since the impact of this change extends beyond tests, I would suggest a more descriptive title.
Something like:
chore: avoid exporting components to the components package

@davidsbatista davidsbatista changed the title fix: make components import in test as specific as possible chore: avoid exporting components to the components package Jan 3, 2025
@davidsbatista davidsbatista merged commit 6e39f5b into main Jan 3, 2025
15 checks passed
@davidsbatista davidsbatista deleted the fixing-init-and-test-imports branch January 3, 2025 14:41
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