-
Notifications
You must be signed in to change notification settings - Fork 94
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
[Package] Changing source directory name #102
Conversation
from rich.console import Console | ||
from rich.layout import Layout | ||
from rich.panel import Panel |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we remove this in this PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unused imports. But I agree this should be separate.
@@ -23,10 +22,10 @@ | |||
from rich.console import Console | |||
|
|||
|
|||
from src.pydantic_models.config_model import Config | |||
from src.utils.save_utils import DirectoryHelper | |||
from src.finetune.finetune import Finetune |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we need this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unused imports. But I agree this should be separate.
from rich.console import Console | ||
from rich.table import Table | ||
from rich.live import Live |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same question: I would keep renaming as one change and fixing rich as another PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed
import statistics | ||
from src.qa.qa_tests import * | ||
from llmtune.qa.qa_tests import * |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please. do not do this. import only what you need
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- This has been resolved with Vivek
- We restructured the design pattern to eliminate having to import here at all
src
to something more descriptive #101