-
Notifications
You must be signed in to change notification settings - Fork 4.7k
Added my Dapt Code #273
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
base: main
Are you sure you want to change the base?
Added my Dapt Code #273
Conversation
Summary of ChangesHello @Vaibhav-03, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly expands the project's capabilities by introducing a complete pipeline for Domain-Adaptive Pretraining (DAPT) of large language models on financial data, specifically Llama 3.1 on earnings call transcripts. It also delivers a robust framework for evaluating various trading strategies, including both long-only and long-short approaches, against a set of established baselines. The changes include new scripts for DAPT training and evaluation, a modular Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
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.
Code Review
This pull request introduces a substantial amount of new functionality, including DAPT model training scripts and a comprehensive evaluation framework. The addition of the evaluation framework is a great step towards systematically measuring performance. I also appreciate the improvements in configuration management, such as using .env files for API keys.
However, there are several critical issues that need attention. The most significant is the large-scale code duplication between the evaluation and evaluation_long_short directories, which will create a maintenance burden. I strongly recommend refactoring this to eliminate redundancy. Additionally, I've found hardcoded secrets and absolute paths in the new scripts, which pose security and portability risks. There are also some instances of risky coding practices, like suppressing all exceptions, that should be addressed. My detailed comments provide specific suggestions for these points.
| from .baseline_strategies import ( | ||
| BuyAndHoldStrategy, | ||
| MACDStrategy, | ||
| KDJRSIStrategy, | ||
| ZMRStrategy, | ||
| SMAStrategy, | ||
| get_all_baseline_strategies | ||
| ) | ||
|
|
||
| from .metrics import ( | ||
| calculate_cumulative_return, | ||
| calculate_annualized_return, | ||
| calculate_sharpe_ratio, | ||
| calculate_maximum_drawdown, | ||
| calculate_all_metrics, | ||
| create_comparison_table | ||
| ) | ||
|
|
||
| from .backtest import ( | ||
| BacktestEngine, | ||
| TradingAgentsBacktester, | ||
| load_stock_data | ||
| ) | ||
|
|
||
| from .visualize import ( | ||
| plot_cumulative_returns, | ||
| plot_transaction_history, | ||
| plot_metrics_comparison, | ||
| plot_drawdown, | ||
| create_summary_report | ||
| ) | ||
|
|
||
| from .run_evaluation import run_evaluation | ||
|
|
||
| __all__ = [ | ||
| # Strategies | ||
| 'BuyAndHoldStrategy', | ||
| 'MACDStrategy', | ||
| 'KDJRSIStrategy', | ||
| 'ZMRStrategy', | ||
| 'SMAStrategy', | ||
| 'get_all_baseline_strategies', | ||
|
|
||
| # Metrics | ||
| 'calculate_cumulative_return', | ||
| 'calculate_annualized_return', | ||
| 'calculate_sharpe_ratio', | ||
| 'calculate_maximum_drawdown', | ||
| 'calculate_all_metrics', | ||
| 'create_comparison_table', | ||
|
|
||
| # Backtesting | ||
| 'BacktestEngine', | ||
| 'TradingAgentsBacktester', | ||
| 'load_stock_data', | ||
|
|
||
| # Visualization | ||
| 'plot_cumulative_returns', | ||
| 'plot_transaction_history', | ||
| 'plot_metrics_comparison', | ||
| 'plot_drawdown', | ||
| 'create_summary_report', | ||
|
|
||
| # Main evaluation | ||
| 'run_evaluation', | ||
| ] No newline at end of file |
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 entire evaluation_long_short directory appears to be a copy of the evaluation directory. This massive code duplication is a critical maintainability issue. Any bug fix or enhancement will need to be applied in two places, which is error-prone and inefficient. Please refactor this to avoid duplication. The different backtesting logic (long-only vs. long-short) could be handled more elegantly, for instance by:
- Passing a strategy type (
'long_only'or'long_short') as a parameter to the backtesting engine. - Using subclassing for different strategy types.
| os.environ["HF_DATASETS_CACHE"] = os.path.join(TMP_DIR, "datasets_cache") | ||
| os.environ["HF_HUB_DISABLE_TELEMETRY"] = "1" | ||
|
|
||
| os.environ["HF_TOKEN"] = "hf_xxx" # replace with your HF token |
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.
Hardcoding placeholder tokens in the code is a security risk. Please load this from an environment variable. This prevents accidental exposure of real tokens.
| os.environ["HF_TOKEN"] = "hf_xxx" # replace with your HF token | |
| os.environ["HF_TOKEN"] = os.getenv("HF_TOKEN") # replace with your HF token |
| os.environ["HF_DATASETS_CACHE"] = os.path.join(TMP_DIR, "datasets_cache") | ||
| os.environ["HF_HUB_DISABLE_TELEMETRY"] = "1" | ||
|
|
||
| os.environ["HF_TOKEN"] = "hf_token" |
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.
| "python": platform.python_version(), | ||
| }) | ||
|
|
||
| from datasets import load_datasets |
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.
| # %% | ||
| from huggingface_hub import InferenceClient | ||
|
|
||
| client = InferenceClient("meta-llama/Llama-3.1-8B", token="hf_xxx") # replace with your HF token |
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.
Hardcoded secrets, even placeholders like "hf_xxx", should not be committed to the source code. This is a security risk as it could be accidentally replaced with a real token. It's better to load secrets from environment variables or a configuration file that is not checked into version control. I see that tradingagents/default_config.py now uses python-dotenv, which is a great pattern to follow here as well.
| client = InferenceClient("meta-llama/Llama-3.1-8B", token="hf_xxx") # replace with your HF token | |
| client = InferenceClient("meta-llama/Llama-3.1-8B", token=os.getenv("HF_TOKEN")) # replace with your HF token |
| except Exception: | ||
| pass |
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 try...except Exception: pass block silently swallows all exceptions. This is risky as it can hide parsing errors or unexpected changes in the API response format, making debugging very difficult. At a minimum, you should log the exception to have a record of when and why parsing failed.
| except Exception: | |
| pass | |
| except Exception as e: | |
| import logging | |
| logging.warning(f"Failed to parse structured output from API response: {e}") |
|
|
||
| from datasets import load_datasets | ||
| from typing import Optional | ||
| import pandas as pds |
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.
| import warnings | ||
| import json | ||
|
|
||
| warnings.filterwarnings('ignore') |
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.
|
|
||
| @staticmethod | ||
| def _actions_to_position(actions: pd.Series) -> pd.Series: | ||
| """Convert action series to a long-only position series in {0,1}.""" |
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.
The docstring is incorrect. It states that this method converts actions to a "long-only position series in {0,1}", but the implementation actually supports long (1), short (-1), and flat (0) positions. The docstring should be updated to accurately reflect the function's behavior.
| """Convert action series to a long-only position series in {0,1}.""" | |
| """Convert action series to a position series in {-1, 0, 1} for long/short/flat.""" |
| import json | ||
|
|
||
| # Add parent directory to path | ||
| sys.path.insert(0, str(Path(__file__).parent.parent)) |
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.
Modifying sys.path manually is generally considered a code smell and can lead to import issues, especially in larger projects. A better approach is to structure the project as a proper Python package (e.g., with a pyproject.toml file). This allows you to install the project in editable mode (pip install -e .), which handles Python's path correctly and makes imports more reliable.
Please share the changes or any issues u observe