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

Add validation to vn.train() inputs #232

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

Conversation

samoliverschumacher
Copy link

This PR adds another input argument checking to vn.train(). If more than one one input is used (question and sql, documentation, ddl, plan), a validation error will be raised that notifies the user.

The Problem

  • If more than one argument is sent to vn.train(), only one item will be sent to the database for later referencing
  • The user wont know which one did, and since no warning is raised, won't know their mistake.

Context

  • vn.train() accepts multiple kwargs for each training item, which are by default None. (question and sql, documentation, ddl, plan).
  • If sql and question are not both None, or not None, a ValidationError exception is raised.
  • If (sql, question) and any other of documentation, ddl, plan, are given, only one will be added, in this order: documentation, sql & question, ddl, plan (which allows for all or some).
vn = VannaDefault()
vn.train(ddl="[EmployeeId] INTEGER PRIMARY KEY AUTOINCREMENT NOT NULL,
                      [LastName] NVARCHAR(20)  NOT NULL,",
         documentation="The 'employees' table contains the FirstName and LastName of employees")
# only documentation would be added

The solution

  • Refactored out the current Validation, and added another check for when more than one is not None.
class VannaBase:
    ...
    @staticmethod
    def _validate_train_args(
        question: Optional[str] = None,
        sql: Optional[str] = None,
        ddl: Optional[str] = None,
        documentation: Optional[str] = None,
        plan: Optional[TrainingPlan] = None,
    ) -> None:
    # Implementation here ...
    ...
    def train(
        self,
        question: str = None,
        sql: str = None,
        ddl: str = None,
        documentation: str = None,
        plan: TrainingPlan = None,
    ) -> str:
    ...
        self._validate_train_args(question, sql, ddl, documentation, plan)

Tests;

  • Followed steps as per CONTRIBUTING.md. Tests pass.

@samoliverschumacher samoliverschumacher marked this pull request as draft February 5, 2024 11:47
@samoliverschumacher samoliverschumacher marked this pull request as ready for review February 5, 2024 12:53
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

1 participant