✨ docker-compose support & new installation method
#212
Conversation
…recommended commit: 3503fdb
Rikj000
changed the title
docker-compose support & new installation method
There was a problem hiding this comment.
First of all, thank you @Bloodsucker for this PR! 🙏
I'm sure a lot of users will be happy with the ability to use docker installations again once this PR is merged 😄
I do have a lot of feedback & some changes that I have to request upon this PR though (See in-code comments)
Hope they won't scare you off & I'm willing to help out with programming on this PR after I finish up issue: #206
| @@ -577,8 +578,9 @@ class MGMHurry: | |||
|
|
|||
| self.logger.info(f'👉 Downloading candle data ({tickers}) for timerange ({timerange})') | |||
|
|
|||
| command = (f'{self.monigomani_config.config["ft_binary"]} download-data --timerange {timerange} ' | |||
| f'-t {tickers} {self.monigomani_config.command_configs()}') | |||
| command = f'{self.monigomani_config.get_freqtrade_cmd()} download-data' | |||
There was a problem hiding this comment.
f-strings can be appended to each other without command +=.
A cleaner implementation would be:
command = (f'{self.monigomani_config.get_freqtrade_cmd()} download-data'
f' --timerange {timerange} -t {tickers} {self.monigomani_config.command_configs()}')There was a problem hiding this comment.
I usually would have prefer this style:
command = ' '.join([
self.monigomani_config.config["ft_binary"],
'download-data',
f'--timerange {timerange}',
f'-t {tickers}',
self.monigomani_config.command_configs()
])I believe this is a discussion over a personal style, however I will honor yours since you're the creator and maintainer.
There was a problem hiding this comment.
It's indeed a discussion of style,
I would agree upon your style suggestion for smaller projects, because it does give a very clean code readability!
And as a + it also eliminates the chance of getting a space too little or too many in the commands. 👏
However I appreciate you sticking to my suggestion, for MGM I prefer this writing because it's already becoming a rather large project & this will keep the amount of lines significantly lower while code readability remains okay 🙂
(We check with flake8 against line lengths not going too long)
| @@ -0,0 +1,3 @@ | |||
| [submodule "freqtrade"] | |||
There was a problem hiding this comment.
I'm fan of the Freqtrade repo submodule!
Didn't knew that was possible with git but it seems to be perfect for embedding the Freqtrade repo in the MoniGoMani repo in a clean way! 🎉
There was a problem hiding this comment.
It's a great feature, but it must be used with caution (it shouldn't be abused). I think this is an excellent example of when to use it! I'm glad this was relevant :)
There was a problem hiding this comment.
Yeah I'll have to take a good in-depth read in the docs on how to manage submodules once we're about to merge this PR!
Have to make sure I know how to use it right so I don't accidentally update the submodule without it being my intention 😄
| hyperopt: | ||
| epochs: 1000 | ||
| loss: MGM_WinRatioAndProfitRatioHyperOptLoss | ||
| stake_currency: USDT | ||
| spaces: buy sell | ||
| strategy: MoniGoManiHyperStrategy | ||
| install_type: source | ||
| mgm_config_names: |
There was a problem hiding this comment.
I don't understand why we would want to deprecate the mgm_config_names section in .hurry?
This currently gives users the ability to easily switch between config files that co-exist in the user_data folder,
examples could be mgm-config-usdt.json & mgm-config-ust.json or mgm-config-private-binance.json & mgm-config-private-kucoin.json.
That's a nice feature to have in my opinion.
I would even agree about moving all .hurry setting besides the mgm_config_names section & install_type from .hurry => mgm-config,
That way we would unify the settings which users configure manually often more which should lead to less confusion about where certain settings are.
(Most users don't know their username can be altered in .hurry for example)
The new proposed "minimal" .hurry format would be of following format then:
config:
install_type: source
mgm_config_names:
mgm-config: mgm-config.json
mgm-config-hyperopt: mgm-config-hyperopt.json
mgm-config-private: mgm-config-private.json(Then we can also scrap the double exchange & stake_currency settings, which currently are defined in both mgm-config & .hurry)
However I suggest we keep this for a separate issue!
There was a problem hiding this comment.
This is going to be a bit controversial but I hope it's open for discussion. Allow me to elaborate my reasons to make that decision as well as to cover on this reply your other comment with my reasons to add the new command use_configuration to the framework.
The main reason on to why I had to deprecate the mgm_config_names from the .hurry file was because Docker can't access the .hurry file. In fact, the only directory FQ has access to are the contents of the user_data directory, which is part of the I/O files that FQ requires to run. If you look at the docker-compose file (from FQ instructions) you'll notice that Docker will mount a volume to access the strategy files. One could add more volumes to the container, but it'll just make more complicated architecture IMHO.
My proposal:
- Follow similar paradigm that FQ uses. Make the strategy folder user_data static, on that directory and simply load whatever is there.
- Change the process when the user needs to "install" a new configuration, located somewhere on his FS, by using the new
use_configurationcommand.
My goal with this approach is to clean-up what I consider FQ files, from what I consider files for the MGM framework, then temporary files/output and the (possible several) user's strategy configuration files. This makes sense if you use a git repo, or a external folder somewhere in your FS, to store your strategy config files without polluting the MGM git repo. My goal for a clean process is: clone the official repo, install, configure framework with my config files and then run. Placing all my data inside the user_data folder doesn't allow me to store the config in a git repo without moving them around each time I modify them.
Following your example from your comment, the user will have a folder my_mgm_configurations anywhere and subfolders that contain all files that make a strategy (the config files!). And this is swapped by the use_cofiguration command. Does that make sense?
To be honest, I see the .hurry more like a "environment configuration file" where you define how you've installed FQ or other meta stuff that is automatically access by MGM framework. But not things that are related to a certain configuration, that belongs to the config files IMO and that can be swapped easily using the use_configuration command.
There was a problem hiding this comment.
I do agree with .hurry being an "environment configuration file",
that's also why I proposed the new "minimal" .hurry format above which would remove all Strategy/Framework related config settings from it.
And then move them to the main mgm-config file where they would be placed more appropriately! 🙂
And if it's about .hurry & mounting with docker then I propose one of 2 following changes:
- Move
.hurryfrom the rootFreqtrade-MGM/folder toFreqtrade-MGM/user_data/,
that way it would be accessible by docker since it'll be in the existing volume.
(Leaning more towards this option sinceuser_data/is Freqtrade's official folder to store used config files, so it would group the config files in a more logical way) - Create a new
volumefor.hurryin ourdocker-compose.yml,
that way it also would be accessibly by docker, but as you mentioned this complicates thedocker-compose.ymlsome more.
However I'm a bit in disagreement with the scrapping the ability to configure custom config names (aka the mgm_config_names section in .hurry),
and the use_configuration implementation that you propose. Since I believe it will lead to more confusion from an end user perspective.
Allow me to elaborate the concerns I have:
user_data/is Freqtrade's official folder to store used config files,
Freqtrade users will be used to it & look for their config files in that folder instead ofuser_data/my_mgm_configurations/- Imagine a user has following config files stored under
user_data/my_mgm_configurations/:-
mgm-config-usdt.json -
mgm-config-ust.json -
mgm-config-tusd.json -
mgm-config-private-binance.json -
mgm-config-private-kucoin.jsonThen the user executes
mgm-hurry use_configurationand picksmgm-config-usdt.json&mgm-config-private-binance.json,
they are copied over touser_data/and renamedmgm-config.json&mgm-config-private.json.Imagine that at this point the user doesn't use MGM for a few months & comes back,
however they forget which files they picked when usingmgm-hurry use_configurationand since this implementation renamed the config files,
they can't find out anymore which config files ofuser_data/my_mgm_configurations/are currently in use without opening the files & comparing them against each other with adiffchecker. Many end users won't know how to do that.
-
- Imagine the user has config changes in
mgm-config.jsonand then executesmgm-hurry use_configuration,
this will lead tomgm-config.jsonbeing overwritten and might lead to lost config changes in some occasions,
unless we implement extra logic to prompt the user if he wants to backup his current config files under a new name before overwriting.
That's why I'd like to keep the mgm_config_names section in .hurry since compared to the solution you suggest it'll have following benefits:
user_data/is Freqtrade's official folder to store used config files, so more users will find the config files without troubles- Imagine a user has following config files stored under
user_data/:-
.hurry(Assuming we'll roll with moving.hurrytouser_data/) -
mgm-config-usdt.json -
mgm-config-ust.json -
mgm-config-tusd.json -
mgm-config-private-binance.json -
mgm-config-private-kucoin.jsonThen the user executes
mgm-hurry switch_configurationand picksmgm-config-usdt.json&mgm-config-private-binance.json,
the names will be stored in.hurryundermgm_config_names.Imagine that at this point the user doesn't use MGM for a few months & comes back,
and again they forgot which config files they picked. Now they can easily check the contents of.hurryto find out which config files are currently in use 🙂
-
- Imagine the user is using and has config changes in
mgm-config-usdt.json, then executesmgm-hurry switch_configurationand picksmgm-config-ust.json.
This will simply switch the used configuration file without any overrides being done, so there also won't be any chance for losing config changes!
I really do appreciate thorough thinking through that you're doing for this issue here!
But I hope you can also agree upon my proposal after clarifying my concerns with your suggestion. 🙂
| @@ -1,16 +1,11 @@ | |||
| config: | |||
| username: MoniGoMani Community | |||
| exchange: binance | |||
| ft_binary: source ./.env/bin/activate; freqtrade | |||
There was a problem hiding this comment.
I'm okay with deprecating ft_binary as long as it doesn't deprecate the ability to use the mgm-hurry command from everywhere when a shell alias has been set!
There was a problem hiding this comment.
With the implementation of PR #224 we should easily be able to deprecate the ft_binary now 🙂
| @@ -1098,6 +1094,44 @@ class MGMHurry: | |||
| message=f'🚀 Fresh **{strategy}** SpreadSheet (.csv) Results ⬇️', | |||
| results_paths=[output_file_path]) | |||
|
|
|||
| def use_configuration(self, config_ho: str = None, config: str = None, config_private: str = None, override: bool = False) -> None: | |||
There was a problem hiding this comment.
I do like the idea for the new mgm-hurry use_configuration command!
However I would do the implementation of it quite differently:
- Rename the command to
mgm-hurry switch_configuration - Remove the mandatory
--overridemodifier (Default intention would be to simply switch between existing configs instead of overriding) - Add an optional
--newmodifier:- Defaults to
False - If
Trueis passed, allow creation of new or overriding existingmgm-config&mgm-config-privatefiles based onmgm-config.example&mgm-config-private.example,
while skipping the interactive list prompts by passing aconfig-name
- Defaults to
- For the functionality of the other 3 optional modifiers (
--config,--config_ho&--config_private):- Don't pass the full path, only pass the
config-name(With or without.json, append.jsonif not provided by user), since all config files will exist inuser_data - If nothing is passed, launch an interactive list prompt to allow picking of
.jsonfile existing inuser_data+ additionalnew config fileoption.- Selecting
new config filelaunches an interactive text prompt, allowing you to fill in a config name (With or without.json, append.jsonif not provided by user)
- Selecting
- If a
config-nameis passed without--newin the command,
check if existing & automatically use (skip the corresponding interactive list prompt), if not existing also launch interactive list prompt. - If a
config-nameis passed with--newin the command, automatically use (skip the corresponding interactive list prompt) - If
--newis passed ornew config filewas selected, check if config file already exists:- Create
config-nameconfig file with a new copy ofmgm-config.exampleormgm-config-private.exampleif not existing - Warn with interactive yes/no prompt if it's okay to overwrite the existing config file with a new copy of
mgm-config.exampleormgm-config-private.example
- Create
- Update corresponding config name in the
mgm_config_namessection of.hurry - Wildcard names for these files would be very nice since it would allow us to auto filter the interactive list prompts!
Then we can make it so they only show the correct config file types in their selection, some examples:- All new
mgm-configfiles will be pre-fixed withmgm-config-, example: user fills inusdt, new file will be calledmgm-config-usdt.json - All new
mgm-config-privatefiles will be pre-fixed withmgm-config-private-, example: user fills inkucoin.json, new file will be calledmgm-config-private-kucoin.json - All new
mgm-config-hyperoptfiles will be pre-fixed withmgm-config-hyperopt-, example: user fills inust-binance, new file will be calledmgm-config-hyperopt-ust-binance.json
- All new
- Don't pass the full path, only pass the
There was a problem hiding this comment.
Please refer to the other (very) long reply: #212 (comment)
I like your proposal of renaming the command, but the command makes sense if different the configuration files aren't directly in the user_data folder. Refer to the other comment for more info on this.
| f'not found in {self.basedir}/.env/bin/freqtrade.')) | ||
| f'not found.')) | ||
|
|
||
| if self.install_type == 'custom' and self.freqtrade_binary: |
There was a problem hiding this comment.
Since we only log here, we can replace these 3 if silent is False: checks with a single if silent is False: check.
There was a problem hiding this comment.
Care to explain how does the silent mode works? Thanks!
There was a problem hiding this comment.
The optional silent parameter can be used to check if Freqtrade and/or MGM are installed through the self.freqtrade_cli.installation_exists(silent=True) and self.monigomani_cli.installation_exists(silent=True) functions without logging to the user about it.
By default these functions would log output to the user which is not always what we want 🙂
For example in the mgm-hurry up command:
| cmd = f'{self.basedir}/freqtrade/.env/bin/freqtrade' | ||
|
|
||
| return cmd | ||
| elif self.config['install_type'] == 'custom': |
There was a problem hiding this comment.
I'm curious which other custom installation types you can think off?
If none then we can perhaps simply drop custom installation types? 🙂
There was a problem hiding this comment.
Yeah... the use isn't really clear 😃 . However, I wanted to use it as a replacement for the .hurry install_fq. Think on a situation where the environment is customized by the user. E.g. FQ is installed in a different location in the system and it doesn't follow the recommended/supported/default MGM framework installation. If removed, with the current changes the user wouldn't be able to customize FQ command with its own.
There was a problem hiding this comment.
I think in the end the process of installation will be very similar for the end user as it currently is:
- Do the initial installation through the web installer with:
/usr/bin/env sh <(curl -s "https://raw.githubusercontent.com/Rikj000/MoniGoMani/development/installer.sh") - Do further updates with
mgm-hurry install_freqtrade&mgm-hurry install_mgm
(Now that I think more about this, perhaps we should rename the commands tomgm-hurry update_freqtrade&mgm-hurry update_mgm, since they will only be used once for installation (Automatically called throughmgm-hurry upin the web-installer). But they will always be used for updating after that, by directly calling them asmgm-hurry ...)
Because the web installer allows for installation in a custom folder & with a custom installation folder name I think we should be able to drop the custom installation type 🙂
I'm still fine with dropping the ft_binary as long as MGM won't have issues detecting the current working directory without it.
Since users can simply run the web installer once again if they wish to move/rename their installation.
| @@ -0,0 +1,30 @@ | |||
| --- | |||
| version: '3' | |||
| services: | |||
There was a problem hiding this comment.
Am I right that we're currently only dockerizing freqtrade & not yet mgm-hurry itself?
Or will docker build -t fq:mgm-recommended . build a container containing both based on the local folder structure?
I'm also wondering if mgm-hurry install_freqtrade & mgm-hurry install_mgm will be able to update their own docker containers?
Or will docker users be forced to re-use the installer.sh script for updating?
(Which normally was only intended for an initial web-installation, after that users update through mgm-hurry itself)
There was a problem hiding this comment.
Am I right that we're currently only dockerizing freqtrade & not yet mgm-hurry itself?
Or will docker build -t fq:mgm-recommended . build a container containing both based on the local folder structure?
Currently only FQ is dockerized but it's possible to run both MGM and FQ inside their own containers (each on their own image). Docker-composer allows to easily manage a set of containers and run "two services" with dependencies and thanks to the Docker magic communicate with each other. It's possible but I've never done it myself.
I'm also wondering if mgm-hurry install_freqtrade & mgm-hurry install_mgm will be able to update their own docker containers?
To update to a newer MGM version, the MGM framework commands should simply checkout the MGM git commit (which will also update the FQ submodule) or checkout a new commit/tag (tag, please!) in the the FQ submodule. Then build. Both Dockerized or source code installation will be automatically updated.
Or will docker users be forced to re-use the installer.sh script for updating?
(Which normally was only intended for an initial web-installation, after that users update through mgm-hurry itself)
This is a good question, but if they already have a working MGM the commands should just checkout to a newer commit version and then build. No need to run external scripts IMO.
There was a problem hiding this comment.
Am I right that we're currently only dockerizing freqtrade & not yet mgm-hurry itself?
Or will docker build -t fq:mgm-recommended . build a container containing both based on the local folder structure?Currently only FQ is dockerized but it's possible to run both MGM and FQ inside their own containers (each on their own image). Docker-composer allows to easily manage a set of containers and run "two services" with dependencies and thanks to the Docker magic communicate with each other. It's possible but I've never done it myself.
I'm a fan of implementing 2 separate services in the docker-compose.yml, it would be a cleaner way of doing things!
I've recently done this with Flagsmith, where they deploy a postgres service for the DataBase and a flagsmith service for the front-end which is connected to the postgres service.
Perhaps we can use their docker-compose.yml as an example.
However I do fear that it might complicate the implementation since we'll have to make sure that the mgm-hurry service will be able to run commands from the freqtrade service without any issues.
Which wouldn't be an issue if we simply run both mgm-hurry and freqtrade in a single container service.
I'm also wondering if mgm-hurry install_freqtrade & mgm-hurry install_mgm will be able to update their own docker containers?
To update to a newer MGM version, the MGM framework commands should simply checkout the MGM git commit (which will also update the FQ submodule) or checkout a new commit/tag (tag, please!) in the the FQ submodule. Then build. Both Dockerized or source code installation will be automatically updated.
Hmmm I'm not such a fan of automatically updating Freqtrade when you update MGM 🧐
Of course most of the times it'll be the recommended thing to do!
(If MGM updates Freqtrade, then the end user will probably have to update his Freqtrade too to remain compatibility)
However this can also lead to confusion for the end user since it won't be clear that his Freqtrade also updated when he updated MGM.
So I propose the following when updating MGM:
- Temporarily store the Freqtrade tag/commit used
- Update MGM (which will automatically update Freqtrade)
- Check if the Freqtrade tag/commit was updated during the installation
- If the Freqtrade tag/commit was updated, prompt the user if he'd like to update to the newer Freqtrade recommended by MGM
- If the answer was yes, finish installation (Freqtrade was already updated when updating MGM)
- If the answer was no, roll Freqtrade back to the temporarily stored Freqtrade tag/commit at the 1st step
- If the Freqtrade tag/commit was updated, prompt the user if he'd like to update to the newer Freqtrade recommended by MGM
Or will docker users be forced to re-use the installer.sh script for updating?
(Which normally was only intended for an initial web-installation, after that users update through mgm-hurry itself)This is a good question, but if they already have a working MGM the commands should just checkout to a newer commit version and then build. No need to run external scripts IMO.
In my answers above I assumed that the docker containers will be able to run the mgm-hurry install_freqtrade & mgm-hurry install_mgm commands.
However I do have a concern that they won't have enough permission to alter the contents of the whole local MGM repo.
As we've talked about before, currently the docker-compose.yml provided by Freqtrade only mounts the user_data/ folder as an accessible volume, but we might have to mount the whole MGM repo folder as a volume to be able to do this.
I'm not certain if this will or won't be needed though, so it'll have to be tested.
|
Ah & FYI: |
|
Hey, thank you very much for having the patience and the diligence to discuss this topic. I've replied to all your comments but before I proceed with further changes a discussion need to occur. I'll be more than happy to slowly add more changes for the community :) |
I'm actually glad that someone is taking the time to properly discuss this with me instead of just asking for docker support 😄 |
|
Linking to PR: #270 |
I'm happy to share some work I've done on this amazing project. There are still pieces that need attention before merging, but the base is ready. Originally the changes were meant to solve the issues I was having with the framework on my system (MacOS and Raspberry Pi), but after discussing with you I thought on making them public for everyone to enjoy. This changes simplify, to my opinion, how the MGM framework and Freqtrade are supposed to be installed and also bring back support for Docker (docker-compose). The priorities of the project that were maintained:
Major changes:
mgm_config_namesin the .hurry. The strategy will always load from the user_data folder.$ python3 mgm-hurry use_configuration --config_ho ./path/to/mgm-config-hyperopt.json --config ./path/to/mgm-config.json --config_private ./path/to/mgm-config-private.json --override(override flag is mantatory, otherwise the command fails).ft_binaryin the .hurry. Accepted values forinstall_typearedocker-compose,source(installation through source code) andcustom. It's possible this property makes sense if used withcustom.What's left to be done in this PR?
Before any further changes, there needs to be a discussion on what to see in here.
setup.shfrom Freqtrade.docker build -t fq:mgm-recommended ..setup.shfrom Freqtrade.docker build -t fq:mgm-recommended ..custominstallation type in the .hurry file needs work, but it's also very simple.Development
I would appreciate kind words, but I'm very open to constructive feedback!