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

Merge extended_conditions into main repository #35

Open
wants to merge 185 commits into
base: dev
Choose a base branch
from

Conversation

RLKRo
Copy link
Member

@RLKRo RLKRo commented Nov 9, 2022

Checklist

  • Add README.md

  • Sync dependencies with df_stats (stats requires httpx<=0.23.0 and requests>=2.28.1 while extended_conditions requires httpx>=0.23.0 and requests<=2.28.1)

  • Skip tests if required dependencies are not installed, env vars are not set or docker is not running

  • Add try: except ImportError when importing extended_conditions dependencies

  • Fix huggingface tests:

    • There were two pytests fixtures named testing_model, I renamed the second one to hf_matcher
    • HFMatcher class requires dataset parameter
  • Replace _extended_conditions_utils.py imports when All example utils moved to dff #34 is merged

  • At the moment rasa docker image building includes model training. This doesn't seem like good practice.

kudep and others added 12 commits October 24, 2022 18:06
- Replace df_engine with dff.core.engine
- Replace df_runner with dff.core.pipeline
- Replace df_extended_conditions with dff.script.logic.extended_conditions
Fix references to files inside examples/extended_conditions since those were moved from examples.

Also rename example_utils.py to _extended_conditions_utils.py.
Also add a step that generates a gdf_account.json file from secret.
Rename the second `testing_model` function to `hf_matcher`
@kudep kudep added this to the Release 0.2.0 milestone Nov 10, 2022
@RLKRo RLKRo added enhancement New feature or request help wanted Extra attention is needed labels Nov 10, 2022
@kudep kudep modified the milestones: Release 0.2.0, Release 0.3.0, Release 0.4.0 Nov 22, 2022
chatsky/conditions/ml.py Outdated Show resolved Hide resolved


@singledispatch
def has_cls_label(model: ExtrasBaseModel, label, threshold: float = 0.9):
Copy link
Member Author

Choose a reason for hiding this comment

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

Add validator for label (present in the model config).

chatsky/conditions/ml.py Outdated Show resolved Hide resolved
chatsky/ml/dataset.py Outdated Show resolved Hide resolved
chatsky/ml/models/base_model.py Show resolved Hide resolved
makefile Outdated Show resolved Hide resolved
chatsky/conditions/ml.py Show resolved Hide resolved
chatsky/core/context.py Outdated Show resolved Hide resolved
chatsky/ml/models/remote_api/async_mixin.py Outdated Show resolved Hide resolved
chatsky/ml/models/remote_api/google_dialogflow_model.py Outdated Show resolved Hide resolved
chatsky/ml/models/remote_api/google_dialogflow_model.py Outdated Show resolved Hide resolved
chatsky/ml/utils.py Outdated Show resolved Hide resolved
chatsky/ml/utils.py Show resolved Hide resolved
chatsky/ml/utils.py Show resolved Hide resolved
def __init__(self, *args):
super().__init__(*args)

async def call(self):
Copy link
Member Author

Choose a reason for hiding this comment

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

Incorrect call signature (should accept context and return bool).

super().__init__(*args)

async def call(self):
async def has_cls_label_innner(ctx: Context, _) -> bool:
Copy link
Member Author

Choose a reason for hiding this comment

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

Remove inner functions.

Comment on lines 81 to 82
if negative_examples is None:
negative_examples = []
Copy link
Member Author

Choose a reason for hiding this comment

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

Use Pydantic's Field default factory instead.

TRANSITIONS: {
# We get to one of the dialog branches depending on the annotation
("service", "buy", 1.2): i_cnd.has_cls_label(
api_model, "LABEL_1", threshold=0.95
Copy link
Member Author

Choose a reason for hiding this comment

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

These models should be passed to pipeline (same as #376) to allow has cls label to accept model name (instead of model instance).

Comment on lines +60 to +78
async def save(self, path: str, **kwargs) -> None:
"""
Save the model to a specified location.

:param path: string-formatted path. If tokenizer state
needs to be saved, the path is used as the base.
"""
raise NotImplementedError

@classmethod
async def load(cls, path: str, namespace_key: str):
"""
Load a model from the specified location and instantiate the model.

:param str: Path to saving directory.
:param namespace_key: Name of the namespace in that the model will be using.
Will be forwarded to the model on construction.
"""
raise NotImplementedError
Copy link
Member Author

Choose a reason for hiding this comment

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

Remove unnecessary methods.

Copy link
Member Author

Choose a reason for hiding this comment

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

Move these files from models/remote_api to models -- there are no other models.

@RLKRo RLKRo mentioned this pull request Nov 1, 2024
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants