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

Make datapilot namespaced #49

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Make datapilot namespaced #49

wants to merge 3 commits into from

Conversation

mdesmet
Copy link
Collaborator

@mdesmet mdesmet commented Feb 12, 2025

Important

Make datapilot a namespaced package and update version management and command registration.

  • Namespace Packaging:
    • Use find_namespace_packages in setup.py to support namespace packages.
    • Add namespace_packages=["datapilot"] in setup.py.
  • Version Management:
    • Move version declaration from src/datapilot/__init__.py to src/datapilot/cli/__init__.py.
    • Update .bumpversion.cfg to reflect the new location of the version string.
  • Command Registration:
    • Add ingest command to datapilot in main.py, with ImportError handling.

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

Copy link

@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 everything up to 277341f in 1 minute and 55 seconds

More details
  • Looked at 46 lines of code in 3 files
  • Skipped 0 files when reviewing.
  • Skipped posting 6 drafted comments based on config settings.
1. setup.py:26
  • Draft comment:
    Good use of find_namespace_packages, but consider verifying all package data is correctly included now that init.py (with version) was removed.
  • Reason this comment was not posted:
    Confidence changes required: 50% <= threshold 50%
    None
2. src/datapilot/cli/main.py:13
  • Draft comment:
    Catching a general ImportError may mask real errors. Consider catching ModuleNotFoundError or logging the exception.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
    The code is clearly implementing optional functionality - if the ingestion module isn't available, it should be skipped silently. Using ImportError is actually more appropriate here because it would catch both missing modules and import-time errors, which is what we want for optional features. ModuleNotFoundError would only catch missing modules, making the error handling less robust.
    The comment raises a valid point about being specific with exception handling. Maybe there are import errors we should be logging.
    In this case, the broad ImportError catch is intentional - we want to silently handle any import-related issues for optional functionality. Logging errors would add noise for an expected condition.
    The current ImportError handling is appropriate for optional feature loading. The comment's suggestion would make the code less robust.
3. setup.py:26
  • Draft comment:
    Using find_namespace_packages is a good update for namespace support; ensure all datapilot subpackages use PEP 420.
  • Reason this comment was not posted:
    Comment looked like it was already resolved.
4. setup.py:28
  • Draft comment:
    Defining namespace_packages ensures proper namespace packaging; verify that expected attributes (e.g. version) are managed elsewhere now.
  • Reason this comment was not posted:
    Marked as duplicate.
5. src/datapilot/__init__.py:1
  • Draft comment:
    Removal of init.py is expected for namespace packages; ensure any required module initialization (e.g. version) is migrated.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
6. src/datapilot/cli/main.py:13
  • Draft comment:
    Silently catching ImportError may hide issues; consider logging a warning for missing optional commands.
  • Reason this comment was not posted:
    Marked as duplicate.

Workflow ID: wflow_kWQZbl0oiwhBV2l6


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

Copy link

@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! Incremental review on 2e2b311 in 1 minute and 4 seconds

More details
  • Looked at 15 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 2 drafted comments based on config settings.
1. src/datapilot/cli/main.py:13
  • Draft comment:
    Catching ImportError silently might hide issues other than optional dependency missing. Consider logging the exception or restricting the import scope.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
    This is a CLI tool where the ingest command is clearly optional. The pattern of silently catching ImportError is standard practice for optional dependencies in Python. Adding logging here could actually be confusing to users since this is an expected case, not an error condition. The code is following the principle of least surprise.
    The comment raises a valid point that catching ImportError broadly could hide real issues. There could be other import-related errors we're masking.
    While true in general, in this specific case we're only importing a single module for an optional CLI command. The risk of masking other issues is very low, and the current approach follows standard Python practices.
    The comment should be deleted. The current code follows standard Python patterns for optional dependencies, and adding error logging would make the behavior more confusing, not better.
2. src/datapilot/cli/main.py:17
  • Draft comment:
    Consider logging the ImportError rather than silently passing it. This will help with debugging when 'ingest' isn’t available.
  • Reason this comment was not posted:
    Marked as duplicate.

Workflow ID: wflow_9ss4HFQH4VP2djix


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

Copy link

@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! Incremental review on b1afbf8 in 1 minute and 5 seconds

More details
  • Looked at 18 lines of code in 2 files
  • Skipped 1 files when reviewing.
  • Skipped posting 4 drafted comments based on config settings.
1. .bumpversion.cfg:14
  • Draft comment:
    Double-check namespacing: bumpversion now looks for version in src/datapilot/cli/init.py. Confirm this change aligns with module structure.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
    The comment is asking the PR author to confirm a change related to namespacing and module structure. It is not making a specific code suggestion or asking for a test to be written. It violates the rule against asking the author to confirm their intention or to double-check things.
2. src/datapilot/cli/__init__.py:1
  • Draft comment:
    Ensure the version value in CLI matches project version and that all references are updated.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
    The rules explicitly state not to make comments that ask the author to verify, ensure, or double-check things. The comment starts with "Ensure that..." which is exactly what we're told to avoid. Additionally, version management is likely handled by the build system or deployment process.
    Perhaps version mismatches could cause serious issues in production and this is a valid concern that should be flagged.
    While version consistency is important, it's typically handled by build systems and CI/CD pipelines. Making a comment asking for verification doesn't add value and violates our rules.
    Delete this comment as it violates the rule against asking authors to verify or ensure things, and version consistency should be handled by automated systems.
3. .bumpversion.cfg:14
  • Draft comment:
    Updated bumpversion reference to the new namespaced init.py location. Ensure other version references (setup.py, docs/conf.py) remain in sync.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50%
    None
4. src/datapilot/cli/__init__.py:1
  • Draft comment:
    Ensure that version stays consistent with bumpversion configuration (0.0.15).
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50%
    None

Workflow ID: wflow_aJWm1CCg002X3Cjk


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

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