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

Contrib: update to LanceDB contrib #533

Merged
merged 3 commits into from
Nov 14, 2023
Merged

Contrib: update to LanceDB contrib #533

merged 3 commits into from
Nov 14, 2023

Conversation

zilto
Copy link
Collaborator

@zilto zilto commented Nov 12, 2023

For existing dataflows -- what has changed?

Added functions for vector and full text search and updated the README and the DAG accordingly.

How I tested this

Used a notebook running all the nodes.

Notes

Checklist

  • PR has an informative and human-readable title (this will be pulled into the release notes)
  • Changes are limited to a single goal (no scope creep)
  • Code passed the pre-commit check & code is left cleaner/nicer than when first encountered.
  • Any change in functionality is tested
  • New functions are documented (with a description, list of inputs, and expected output)
  • Dataflow documentation has been updated if adding/changing functionality.

Copy link
Contributor

sweep-ai bot commented Nov 12, 2023

Apply Sweep Rules to your PR?

  • Apply: All new business logic should have corresponding unit tests.
  • Apply: Refactor large functions to be more modular.


# Configuration Options
This module doesn't receive any configuration.

## Inputs:
## Required Inputs:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why "required"?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I thought that Inputs might imply that is an exhaustive list of all the inputs. Instead, this list includes that you will have to pass as input to use the module albeit not all concurrently

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah I think Inputs is fine. No need to for that extra qualification. Hamilton should complain anyway if something isn't satisfied.

@skrawcz
Copy link
Collaborator

skrawcz commented Nov 13, 2023

Looking good otherwise -- just that one question.

@skrawcz skrawcz merged commit 7178938 into main Nov 14, 2023
18 of 22 checks passed
@skrawcz skrawcz deleted the contrib/lancedb-update branch November 14, 2023 01:24
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

2 participants