Skip to content
This repository has been archived by the owner on Jun 8, 2023. It is now read-only.

🐞 Fix bugs introduced by updating Freqtrade #91

Closed
rodrigogs opened this issue Jul 13, 2021 · 18 comments · Fixed by #99, #146 or #150 · May be fixed by #195
Closed

🐞 Fix bugs introduced by updating Freqtrade #91

rodrigogs opened this issue Jul 13, 2021 · 18 comments · Fixed by #99, #146 or #150 · May be fixed by #195
Assignees
Labels
Bug - Fixed Issue has been resolved
Milestone

Comments

@rodrigogs
Copy link

I just updated my freqtrade image, and now I get this message when I try to hyperopt:

freqtrade - ERROR - This strategy requires 4800 candles to start. Binance only provides 1000 for 5m.

Any idea why?

@Rikj000 Rikj000 added Bug - Fix Needed Something isn't working as expected Planned Planned feature, improvement or bugfix (not being worked on yet) labels Jul 13, 2021
@Rikj000 Rikj000 added this to the v0.13.0 milestone Jul 13, 2021
@Rikj000 Rikj000 changed the title Unable to hyperopt with current development freqtrade Fix bugs introduced by updating Freqtrade Jul 13, 2021
@Rikj000
Copy link
Owner

Rikj000 commented Jul 13, 2021

freqtrade - ERROR - This strategy requires 4800 candles to start. Binance only provides 1000 for 5m.
Any idea why?

Yes, you are on the wrong Freqtrade commit, you currently need an older version of Freqtrade,
as mentioned lately on the front page of the development branch.
Steps for how to change to the recommended specific commit are also on Discord.

It appears TimeFrame-Zoom got broken on newer Freqtrade versions.
It always zooms in now, even when dry/live running, which is not as intended + causing your issues.

This was also mentioned earlier today in the help channel on Discord.

You'll have to wait till I or another willing developer update Freqtrade again and then iron out all the bugs introduced into MGM by doing that.
On my en this will probably happen:

  • Right before wrapping up the next MGM release
  • Could also happen earlier if I want/need a specific & very recently added feature out of Freqtrade for MGM

@rodrigogs
Copy link
Author

Oh, I didn't notice your note in the readme. Thank you, a fast and precise answer as always :)

@topscoder
Copy link
Collaborator

Took a while, but figured out this is the particular commit in freqtrade/freqtrade who is responsible for this issue:

freqtrade/freqtrade@2f33b97

@Rikj000
Copy link
Owner

Rikj000 commented Jul 20, 2021

freqtrade/freqtrade@2f33b97

Good job mate! 🦾 Hmmm this is problematic though... 🤔
The validate_required_startup_candles() function indeed seems to be the issue here 👀
However we cannot modify the execution of this function from at the strategy level I fear..
I believe a small/simple PR to Freqtrade will be needed to resolve this and to allow us to keep using TimeFrame-Zoom.. 🤔
(Just check the RunMode before you execute, an example of how this can be found in the __init__() function in the MasterMoniGoManiHyperStrategy file.)

Because it's ok for Freqtrade to throw this error when you're Dry/Live running, because Binance simply will not allow you to use that many candles during dry/live runs.
But during backtesting/hyperopting this is no issue, because we already downloaded all the candle data prior to our backtesting/hyperopting, so we will not run into Binance API issues.

@topscoder
Copy link
Collaborator

I've hacked a bit around and it seems the startup_candle_count (2400) during hyperopting isn't accepted anymore. But, the default (400) is.

So I've added a check to only change the startup_candle_count in backtest-mode.

MasterMoniGoManiHyperStrategy.py

358: -            self.startup_candle_count *= self.timeframe_multiplier

358: +            if RunMode(config.get('runmode', RunMode.OTHER)) is RunMode.BACKTEST:
358: +                self.startup_candle_count *= self.timeframe_multiplier

@Rikj000
Copy link
Owner

Rikj000 commented Jul 20, 2021

I've hacked a bit around and it seems the startup_candle_count (2400) during hyperopting isn't accepted anymore. But, the default (400) is.

Yes exactly what I meant! 👏 Would you be up for it to describe this issue in a PR to Freqtrade?
Perhaps link this issue too, because we're a good example where this leads to issues 😇

@topscoder
Copy link
Collaborator

I've hacked a bit around and it seems the startup_candle_count (2400) during hyperopting isn't accepted anymore. But, the default (400) is.

Yes exactly what I meant! 👏 Would you be up for it to describe this issue in a PR to Freqtrade?

Uhmm, I think so... But my proposal above is an easy patch inside MasterMoniGoManiHyperStrategy.py. That would cost a lot less effort if I'm guessing right?

@topscoder
Copy link
Collaborator

I'll create a PR containing above patch to close this issue. We can discuss it easier if it's needed.

@Rikj000
Copy link
Owner

Rikj000 commented Jul 20, 2021

Uhmm, I think so... But my proposal above is an easy patch inside MasterMoniGoManiHyperStrategy.py. That would cost a lot less effort if I'm guessing right?

Oh damn I didn't notice! Thought you modified Freqtrades source code 😮

@Rikj000
Copy link
Owner

Rikj000 commented Jul 20, 2021

MasterMoniGoManiHyperStrategy.py

358: -            self.startup_candle_count *= self.timeframe_multiplier

358: +            if RunMode(config.get('runmode', RunMode.OTHER)) is RunMode.BACKTEST:
359: +                self.startup_candle_count *= self.timeframe_multiplier

What is the startup candle count that get's printed to the console with this patch? 🤔
If it gets rid of the bug I believe it will be the "unzoomed" 400 candle startup count that is used?

@topscoder
Copy link
Collaborator

Correct. 400

@Rikj000
Copy link
Owner

Rikj000 commented Jul 20, 2021

Correct. 400

Now that I think more about it, I believe we don't need the multiplier on the startup_candle_count.. 🤔

I found the commit where it got pushed 41f9875
There MoniGoManiHyperStrategy.py file, Line 916: self.startup_candle_count *= self.timeframe_multiplier was added.

I also found HyperOptResults5_Pt1-23-05-2021_MoniGoManiConfiguration_bugfixed_startup_candle_count.log in the Some Test Results/v0.11.0 folder. And I now recall that it was not really an issue back then, but something I believed what was needed for the timeframe zoom implementation to work correctly.

Back then, I thought, if the timeframe zooms in to 5 minutes, then the dataframe timerange will be too short for the larger informative indicators/signals that get added in the dataframe (Because 400*5m startup is shorter then 400*30m startup).

However, if we follow the get_pair_dataframe() function in MasterMoniGoManiHyperStrategy.py, then we end up at Freqtrade's load_pair_history() function, which takes in the startup_candles parameter.

2 things to consider here:

  • No matter if under 5m candles or under 30m or any other candles, the 200EMA (which has the longest startup of the signals used by MGM), will always only need 400 candles as it's startup count.
  • We are never interested in the indicator/signal data of the backtest_timeframe, only in it's intra-candle price movement

After my re-reflection on it, I believe this line (self.startup_candle_count *= self.timeframe_multiplier) as a whole can be removed safely as the proper fix! 🎉

@Rikj000 Rikj000 added In Progress This is being worked on and removed Planned Planned feature, improvement or bugfix (not being worked on yet) labels Jul 20, 2021
@Rikj000 Rikj000 linked a pull request Jul 20, 2021 that will close this issue
@hhqwerty
Copy link

@rodrigogs in case you're using docker, I created an image that use freqtrade commit 3503fdb4 as mentioned in Discord. so you only need to change in docker-compose file : from image: freqtradeorg/freqtrade:develop to hoanghnbk/freqtrade_3503fdb4 and it will work

@rodrigogs
Copy link
Author

I already did the same thing hahaha

Thanks!

topscoder added a commit to topscoder/MoniGoMani that referenced this issue Aug 4, 2021
@Rikj000
Copy link
Owner

Rikj000 commented Aug 14, 2021

Even though PR #99 got merged I truly recommend to not update your Freqtrade!

Please remain on Freqtrade commit freqtrade/freqtrade@3503fdb until instructed otherwise!

Only update your Freqtrade if you wish to:

  • Check for new breakage in MGM that might occur
  • BugFix & PR new breakage if it occurs

@edebie
Copy link

edebie commented Aug 23, 2021

@rodrigogs in case you're using docker, I created an image that use freqtrade commit 3503fdb4 as mentioned in Discord. so you only need to change in docker-compose file : from image: freqtradeorg/freqtrade:develop to hoanghnbk/freqtrade_3503fdb4 and it will work

You also need to update docker/Dockerfile.MoniGoMani

@Rikj000 Rikj000 added Planned Planned feature, improvement or bugfix (not being worked on yet) and removed In Progress This is being worked on labels Aug 25, 2021
@Rikj000 Rikj000 changed the title Fix bugs introduced by updating Freqtrade 🐞 Fix bugs introduced by updating Freqtrade Sep 5, 2021
@Rikj000
Copy link
Owner

Rikj000 commented Sep 13, 2021

☑️ Handle Freqtrade's new way of dealing with protections, appears like they aren't used in the .json files anymore

@Rikj000 Rikj000 linked a pull request Sep 21, 2021 that will close this issue
@Rikj000 Rikj000 linked a pull request Oct 3, 2021 that will close this issue
@Rikj000 Rikj000 moved this from In Progress to Planned in MoniGoMani - Global Development Progress Oct 16, 2021
@Rikj000 Rikj000 self-assigned this Nov 24, 2021
@Rikj000 Rikj000 added In Progress This is being worked on and removed Planned Planned feature, improvement or bugfix (not being worked on yet) labels Dec 18, 2021
@Rikj000 Rikj000 moved this from Planned to In Progress in MoniGoMani - Global Development Progress Dec 18, 2021
@Rikj000
Copy link
Owner

Rikj000 commented Jan 16, 2022

Updated to freqtrade-2021.11 in: 527b106...fdbb526 🎉

@Rikj000 Rikj000 closed this as completed Jan 16, 2022
MoniGoMani - Global Development Progress automation moved this from In Progress to Done Jan 16, 2022
@Rikj000 Rikj000 added Bug - Fixed Issue has been resolved and removed In Progress This is being worked on Bug - Fix Needed Something isn't working as expected labels Jan 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.