Skip to content

Conversation

@batwood-1
Copy link
Collaborator

@batwood-1 batwood-1 commented Dec 9, 2024

Description

Related Issue(s)

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • My code follows the code style of this project.

@batwood-1 batwood-1 requested review from fabiocat93 and removed request for fabiocat93 December 9, 2024 14:56
@batwood-1 batwood-1 self-assigned this Dec 9, 2024
Copy link
Collaborator

@fabiocat93 fabiocat93 left a comment

Choose a reason for hiding this comment

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

hi @batwood-1 , i have left some comments. I feel some changes are needed here. You can start addressing my comments and we can meet early Jan and discuss this merge in person. what do you think?

@fabiocat93 fabiocat93 added the enhancement New feature or request label Dec 23, 2024
@fabiocat93 fabiocat93 marked this pull request as draft December 23, 2024 16:53
@fabiocat93
Copy link
Collaborator

@batwood-1 thank you for addressing some of my comments. We will check this deeply tomorrow in person

@fabiocat93 fabiocat93 mentioned this pull request Jan 22, 2025
Copy link
Collaborator Author

@batwood-1 batwood-1 left a comment

Choose a reason for hiding this comment

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

Fixed all comments, check out latest changes under 'fixed evaluations'

@batwood-1 batwood-1 marked this pull request as ready for review January 30, 2025 01:26
@batwood-1 batwood-1 requested a review from fabiocat93 January 30, 2025 01:28
pyproject.toml Outdated
scikit-learn = "~=1.5"
nltk = "~=3.9"
sacrebleu = "^2.4.3"
pytest-testmon = "^2.1.1"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I suspect you don't want pytest-testmon in production, but maybe only among the dev dependencies. and why do you want this?

pyproject.toml Outdated
umap-learn = "~=0.5"
scikit-learn = "~=1.5"
nltk = "~=3.9"
sacrebleu = "^2.4.3"
Copy link
Collaborator

Choose a reason for hiding this comment

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

For consistency and easier dependency solving, can you use the tilde as for the other dependencies? ~=2.4

pyproject.toml Outdated
vocos = "~=0.1"
deepeval = "~=2.2.6"
rouge-score = "~=0.1.2"
textstat = "^0.7.4"
Copy link
Collaborator

Choose a reason for hiding this comment

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

same as above

pyproject.toml Outdated
select = ["ANN", "D", "E", "F", "I"]
ignore = [
"ANN101", # self should not be annotated.
"ANN102" # cls should not be annotated.
Copy link
Collaborator

Choose a reason for hiding this comment

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

why this? I suspect ANN101 and ANN102 should be there since we never annotate self

Copy link
Collaborator

Choose a reason for hiding this comment

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

this is actually empty

class LLM:
"""Wrapper for invoking various LLMs.
This class provides a unified interface for interacting with LLMs,
Copy link
Collaborator

Choose a reason for hiding this comment

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

is the external server the easiest way to interact with these models? sounds like an extra effort that we are asking to our users

Copy link
Collaborator

Choose a reason for hiding this comment

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

I feel this is more code for the ALI project than code that will should be part of a reusable API

Copy link
Collaborator

Choose a reason for hiding this comment

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

Why cannot llm response be a scriptline?

Copy link
Collaborator

Choose a reason for hiding this comment

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

how much of this and transcript_output is ALI -specific or reusable by others? My general comment is to try split the code that is specific to ALI and the code that you want to make available to everyone in the community

Copy link
Collaborator

@fabiocat93 fabiocat93 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 left comments here and there. I feel that we are going in the right direction, but you may want to think about a more generalizable API for interacting with the llms (not just in the context of the ALI project)

@fabiocat93 fabiocat93 mentioned this pull request Apr 11, 2025
@fabiocat93 fabiocat93 marked this pull request as draft August 29, 2025 13:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Integrate LLMs (with langchain) for conversational script analysis

3 participants