-
Notifications
You must be signed in to change notification settings - Fork 152
✨ docker-compose
support & new installation method
#212
base: development
Are you sure you want to change the base?
✨ docker-compose
support & new installation method
#212
Conversation
…recommended commit: 3503fdb
docker-compose
support & new installation method
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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_configuration
command.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.hurry
from 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
volume
for.hurry
in ourdocker-compose.yml
,
that way it also would be accessibly by docker, but as you mentioned this complicates thedocker-compose.yml
some 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.json
Then the user executes
mgm-hurry use_configuration
and 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_configuration
and 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 adiff
checker. Many end users won't know how to do that.
-
- Imagine the user has config changes in
mgm-config.json
and then executesmgm-hurry use_configuration
,
this will lead tomgm-config.json
being 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.hurry
touser_data/
) -
mgm-config-usdt.json
-
mgm-config-ust.json
-
mgm-config-tusd.json
-
mgm-config-private-binance.json
-
mgm-config-private-kucoin.json
Then the user executes
mgm-hurry switch_configuration
and picksmgm-config-usdt.json
&mgm-config-private-binance.json
,
the names will be stored in.hurry
undermgm_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.hurry
to 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_configuration
and 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
--override
modifier (Default intention would be to simply switch between existing configs instead of overriding) - Add an optional
--new
modifier:- Defaults to
False
- If
True
is passed, allow creation of new or overriding existingmgm-config
&mgm-config-private
files 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.json
if 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
.json
file existing inuser_data
+ additionalnew config file
option.- Selecting
new config file
launches an interactive text prompt, allowing you to fill in a config name (With or without.json
, append.json
if not provided by user)
- Selecting
- If a
config-name
is passed without--new
in the command,
check if existing & automatically use (skip the corresponding interactive list prompt), if not existing also launch interactive list prompt. - If a
config-name
is passed with--new
in the command, automatically use (skip the corresponding interactive list prompt) - If
--new
is passed ornew config file
was selected, check if config file already exists:- Create
config-name
config file with a new copy ofmgm-config.example
ormgm-config-private.example
if 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.example
ormgm-config-private.example
- Create
- Update corresponding config name in the
mgm_config_names
section 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-config
files will be pre-fixed withmgm-config-
, example: user fills inusdt
, new file will be calledmgm-config-usdt.json
- All new
mgm-config-private
files 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-hyperopt
files 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Care to explain how does the silent mode works? Thanks!
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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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://github.com/raw/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 up
in 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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_names
in 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_binary
in the .hurry. Accepted values forinstall_type
aredocker-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.sh
from Freqtrade.docker build -t fq:mgm-recommended .
.setup.sh
from Freqtrade.docker build -t fq:mgm-recommended .
.custom
installation 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!