nautechsystems / nautilus_trader

A high-performance algorithmic trading platform and event-driven backtester
https://nautilustrader.io
GNU Lesser General Public License v3.0
2.12k stars 479 forks source link

Using BinanceFuturesInstrumentProvider during backtests has side effects #1114

Closed Otlk closed 1 year ago

Otlk commented 1 year ago

Bug Report

Using BinanceFuturesInstrumentProvider during backtests as described here and here has side effects.

Maybe due to OrderMatchingEngine:process_bar -> OrderMatchingEngine:_process_trade_ticks_from_bar ?

Expected Behavior

Should process bars and construct / update cached prices

Actual Behavior

Steps to Reproduce the Problem

  1. Using examples/backtest/crypto_ema_cross_ethusdt_trailing_stop.py
  2. Create a BinanceFuturesInstrumentProvider instance

    async def create_provider():
    clock = LiveClock()
    log = Logger(clock=clock)
    
    client = get_cached_binance_http_client(
        loop=asyncio.get_event_loop(),
        clock=clock,
        logger=log,
        account_type=BinanceAccountType.FUTURES_USDT,
        key="a",
        secret="a"
    )
    await client.connect()
    
    binance_provider = BinanceFuturesInstrumentProvider(
        client=client,
        clock=clock,
        logger=log,
        config=InstrumentProviderConfig(load_all=True, log_warnings=False)
    )
    
    await binance_provider.load_all_async()
    await client.disconnect()
    return binance_provider
  3. Change the config to use bars
    
    provider: BinanceFuturesInstrumentProvider = asyncio.run(create_provider())
    instrument_id = InstrumentId(symbol=Symbol("BTCUSDT-PERP"), venue=BINANCE)
    instrument = provider.find(instrument_id)
    engine.add_instrument(instrument)

bar_type = "BTCUSDT-PERP.BINANCE-1-MINUTE-LAST-EXTERNAL" wrangler = BarDataWrangler(bar_type=BarType.from_str(bar_type), instrument=instrument) bars = wrangler.process(TestDataProvider().read_csv_bars("ftx-btc-perp-20211231-20220201_1m.csv")) engine.add_data(bars)

config = EMACrossTrailingStopConfig( instrument_id=str(instrument.id), bar_type=bar_type, trade_size=Decimal("0.05"), fast_ema_period=10, slow_ema_period=20, atr_period=20, trailing_atr_multiple=3.0, trailing_offset_type="PRICE", trigger_type="LAST_TRADE", )



### Specifications

- OS platform: Windows 10
- Python version: 3.11
- `nautilus_trader` version: latest
cjdsellers commented 1 year ago

So I don't believe this one is a bug, but the design differs from your expectations.

When a bar is processed by the MatchingEngine the temporary quote or trade ticks which are created from this are only used to process the internal order book, and are not added to the cache or emitted as data events over the message bus.

This could be added, although is a different path to how some users have been setting things up, where bar data is pre-processed into the desired tick type, and then the desired bar type will be subscribed for and built to match the original bar data. The advantage of this is that the tick data will then follow the normal path through the DataEngine as its part of the main data stream (rather than ticks being inserted directly into the cache which would fix your issue above, but this would then be an internal system flow specific to backtesting).

This isn't documented, but order book and then quote / trade tick data are considered the first class method for using the platform, with bar only backtesting supported although with less functionality as you've found above.

If you look through some of the examples you'll see how bars can be pre-processed into ticks, and then the desired bar type subscribed to:

    # Add data
    wrangler = QuoteTickDataWrangler(instrument=GBPUSD_SIM)
    ticks = wrangler.process_bar_data(
        bid_data=provider.read_csv_bars("fxcm-gbpusd-m1-bid-2012.csv"),
        ask_data=provider.read_csv_bars("fxcm-gbpusd-m1-ask-2012.csv"),
    )
    engine.add_data(ticks)

    # Configure your strategy
    config = EMACrossBracketConfig(
        instrument_id=str(GBPUSD_SIM.id),
        bar_type="GBP/USD.SIM-5-MINUTE-BID-INTERNAL",
        fast_ema_period=10,
        slow_ema_period=20,
        bracket_distance_atr=3.0,
        trade_size=Decimal(1_000_000),
    )

I'll await your thoughts/feedback on this before closing the issue.

Otlk commented 1 year ago

I've seen the example mentioned but wrongly assumed I needed two distinct datasets to process bars this way.

The path you provided for preprocessing this bar data into ticks is perfectly fine for me and seem more suitable than simple bar ingestion.

So changing processing to the suggested method effectivily permit self.portfolio.unrealized_pnls(venue) and self.portfolio.net_exposures(venue) to be calculated. But the post run currency statistics still produce zero results while the returns statistics are correctly calculated:

 -----------------------------------------------------------------
  PnL Statistics (USDT)
 -----------------------------------------------------------------
 PnL (total):                    0.0
 PnL% (total):                   0.0
 Max Winner:                     0.0
 Avg Winner:                     0.0
 Min Winner:                     0.0
 Min Loser:                      0.0
 Avg Loser:                      0.0
 Max Loser:                      0.0
 Expectancy:                     0.0
 Win Rate:                       0.0

Changing the instrument provider from BinanceFuturesInstrumentProvider to TestInstrumentProvider in the config fragment i provided above correcly produce these currencies statistics, any idea why ? Am i missing something here ?

    # This fails to produce currency pnl statistics
    # provider: BinanceFuturesInstrumentProvider = asyncio.run(create_provider())
    # instrument_id = InstrumentId(symbol=Symbol("ETHUSDT-PERP"), venue=BINANCE)
    # instrument = provider.find(instrument_id)

    instrument = TestInstrumentProvider.ethusdt_perp_binance()
    instrument_id = instrument.id
    engine.add_instrument(instrument)

    bar_type = f"{instrument_id.value}-1-MINUTE-BID-INTERNAL"
    wrangler = QuoteTickDataWrangler(instrument=instrument)
    ticks = wrangler.process_bar_data(
        bid_data=TestDataProvider().read_csv_bars("ftx-btc-perp-20211231-20220201_1m.csv"),
        ask_data=TestDataProvider().read_csv_bars("ftx-btc-perp-20211231-20220201_1m.csv")
    )

    engine.add_data(ticks)

    # Configure your strategy
    config = EMACrossTrailingStopConfig(
        instrument_id=str(instrument.id),
        bar_type=bar_type,
        trade_size=Decimal("0.05"),
        fast_ema_period=10,
        slow_ema_period=20,
        atr_period=20,
        trailing_atr_multiple=3.0,
        trailing_offset_type="PRICE",
        trigger_type="LAST_TRADE",
    )
cjdsellers commented 1 year ago

Interesting, my initial impression is that the quote currency is different between those instruments for some reason, which is what is used as the basis to calculate those PnL stats.

Could you please show the following:

Otlk commented 1 year ago

Sure, for information I'm using examples/backtest/crypto_ema_cross_ethusdt_trailing_stop.py and modified it slightly, attached for reference main.zip.

cjdsellers commented 1 year ago

Hi @Otlk

I finally had some bandwidth to look into this, after some digging I uncovered a bug in an unusual place. It actually turned out to the how we handle Currency equality.

Previously we were just driving PartialEq + Eq in Rust, and then simply using a C FFI function to compare two currencies. The issue with this is that Currency actually has 5 fields, and so derived equality is going to be based on all of these. The new Binance implementation actually defines currencies slightly differently when parsing them compared to the built-in currency constants (with the same codes).

So at the end of the backtest during the post-run analysis the currency which produced PnL for the positions was not equal to the currency received from the BinanceInstrumentProvider (even though the currency code ETH etc was exactly the same).

So the fix was to implement PartialEq for Currency and just compare the currency code field (as I think its fine to consider currencies with the same code as equal, these codes should always be unique for correct operation of the platform).

So I just pushed this fix to develop branch, and I've also taken the liberty of using the MRE script you provided as the basis of a new backtest example script, which uses a real BinanceInstrumentProvider to asynchronously load instruments prior to the backtest run.

Thanks for your patience on this one and for the report! I've credited you in the upcoming release notes :pray:

Otlk commented 1 year ago

Thank you for the fix and continuous support !