-
-
Notifications
You must be signed in to change notification settings - Fork 296
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
FEATURE: add csvsource #1389
FEATURE: add csvsource #1389
Conversation
Hi @go-dockly, This is KarmaBot, and we reward your contributions with tokens sent directly to your wallet to support development. This pull request may get 3955 BBG. To receive BBG tokens, please provide your Polygon (can be Ethereum) address as an issue comment in this pull request, following this format:
Once this pull request is merged, your BBG tokens will be transferred to your wallet. -- Best, |
Re-estimated karma: this pull request may get 4072 BBG |
Re-estimated karma: this pull request may get 4126 BBG |
Re-estimated karma: this pull request may get 4152 BBG |
@go-dockly can do you git rebase against the main branch? and maybe squash some fix commits? I saw you have merge-point commits, thanks |
|
Re-estimated karma: this pull request may get 4222 BBG |
c96eb7a
to
b9150e5
Compare
Re-estimated karma: this pull request may get 4228 BBG |
@@ -375,12 +376,12 @@ func (e *Exchange) SubscribeMarketData( | |||
for symbol := range loadedSymbols { | |||
symbols = append(symbols, symbol) | |||
} | |||
symbols = append(symbols, "FXSUSDT") |
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.
debugging code?
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.
I didn't really find a way yet to init the loadedSymbols from config
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.
nor did the backtest execute any trades so I assume the trade service is not properly connected to the backtest exchange. Do you have any ideas what I am missing here?
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.
You have to set it in the config yaml. check pkg/bbgo/config.go
in the Backtest
struct.
backtest:
symbols:
- BTCUSDT
- ETHUSDT
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.
also you need to set the sync
section maybe.
sync:
userDataStream:
trades: true
filledOrders: true
sessions:
- binance
symbols:
- BTCUSDT
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.
gonna try that thanks
pkg/service/backtest_sql.go
Outdated
type BacktestService struct { | ||
DB *sqlx.DB | ||
} | ||
|
||
func (s *BacktestService) SyncKLineByInterval(ctx context.Context, exchange types.Exchange, symbol string, interval types.Interval, startTime, endTime time.Time) error { | ||
func NewBacktestService(db *sqlx.DB) IBacktestService { |
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.
avoid setting the interface type as the return type in the constructor, you can force the interface type from the caller. e.g.,
var s IBacktestService = NewBacktestService(db)
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.
and where would you take db from?
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.
it's initialized from the bbgo.Environment
@go-dockly I saw you're adding more changes to this PR, this could make this PR too large to review as the previous PR issue, could you please narrow down the scope of this PR, and if you need to continue the work, you can create another PR that sets the base branch to this one |
Your original changes was almost ready to be merged, it just needs to be rebased (git rebase) |
case types.ExchangeBybit: | ||
csvContent, err = gunzip(body) | ||
if err != nil { | ||
return nil, fmt.Errorf("gunzip data %s: %w", exchange, err) |
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.
looks like the exchange variable is only used in the log?
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.
switch exchange {
case types.ExchangeBybit:
It wouldn't have worked if we had merged before. Please check the last commit about what I mean addMissingKLines... |
@@ -150,7 +151,8 @@ type Backtest struct { | |||
Sessions []string `json:"sessions" yaml:"sessions"` | |||
|
|||
// sync 1 second interval KLines | |||
SyncSecKLines bool `json:"syncSecKLines,omitempty" yaml:"syncSecKLines,omitempty"` | |||
SyncSecKLines bool `json:"syncSecKLines,omitempty" yaml:"syncSecKLines,omitempty"` | |||
CsvSource *csvsource.CsvConfig `json:"csvConfig,omitempty" yaml:"csvConfig,omitempty"` |
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.
it's better to put the Config struct under the bbgo package (avoid importing csvsource, could cause cyclic import in the future)
@@ -163,7 +162,14 @@ var BacktestCmd = &cobra.Command{ | |||
|
|||
environ := bbgo.NewEnvironment() | |||
|
|||
backtestService := service.NewBacktestServiceCSV(outputDirectory, csvsource.SPOT, csvsource.AGGTRADES) // todo make configurable | |||
if userConfig.Backtest.CsvSource == nil { |
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 should be optional
@@ -20,6 +26,10 @@ type CsvTick struct { | |||
} | |||
|
|||
func (c *CsvTick) ToGlobalTrade() (*types.Trade, error) { | |||
var isFutures bool | |||
if c.Market == FUTURES { |
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.
constants should be in CamelCase (like this)
@@ -5,8 +5,14 @@ import ( | |||
"github.com/c9s/bbgo/pkg/types" | |||
) | |||
|
|||
type CsvConfig struct { | |||
Market MarketType `json:"market"` |
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.
move the MarketType to the types package
func (c *CsvTick) ToGlobalTrade() (*types.Trade, error) { | ||
var isFutures bool | ||
if c.Market == FUTURES { | ||
isFutures = true |
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.
it could be just:
isFutures = c.Market == FUTURES
@@ -0,0 +1,16 @@ | |||
trade_id/���id,side/������,size/����,price/�۸�,created_time/�ɽ�ʱ�� |
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.
what's the text encoding here?
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.
good question not sure what okex is doing here. It's their raw content but we skip this line anyway on ingest
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.
I guess it's chinese, maybe you need to use UTF-8 to decode it?
got it, thanks |
FYI, best practice: rebase while you're git pulling https://sdq.kastel.kit.edu/wiki/Git_pull_--rebase_vs._--merge |
|
||
1: Through csv data source (supported right now are binance, bybit and OkEx) | ||
|
||
2: Alternatively run backtests through [MySQL](../../README.md#configure-mysql-database) or [SQLite3 |
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.
Would suggest to put mysql as the default backtest engine.
Let's put the csv data source as the second one
@go-dockly waiting for you updates ;-) |
@go-dockly I am trying to rebase your commits, and found you have the "reset main" commits, please avoid doing this in the pull request, or it could make it harder to do the rebase you should avoid merge the main branch into your branch, instead you can use |
@go-dockly this is what the "reset main" causes during the rebase process :/ |
I have rebased the commits in this PR #1454 |
@go-dockly please continue your work from the pull request #1454 (branch |
Add a tick and kline level reader interface for multiple exchanges (binance, bybit) or fin-apps like meta-trader.
The goal is to expose the stream interface on the csvsource so arbitrary tick level simulations can be run against strategy