This repository has been archived by the owner on Jun 8, 2023. It is now read-only.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
✨
docker-compose
support & new installation method #212base: development
Are you sure you want to change the base?
✨
docker-compose
support & new installation method #212Changes from all commits
be7b122
3bf6806
bd58082
cb0e9b1
e4a6e89
c90c71b
e8e8fbe
4e7ecbd
4ff410d
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 😄
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 themgm-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 🙂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
ormgm-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 themgm_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:(Then we can also scrap the double
exchange
&stake_currency
settings, which currently are defined in bothmgm-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 theuser_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:
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:.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 since
user_data/
is Freqtrade's official folder to store used config files, so it would group the config files in a more logical way)volume
for.hurry
in ourdocker-compose.yml
,that way it also would be accessibly by docker, but as you mentioned this complicates the
docker-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 of
user_data/my_mgm_configurations/
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 to
user_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 using
mgm-hurry use_configuration
and since this implementation renamed the config files,they can't find out anymore which config files of
user_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.mgm-config.json
and then executesmgm-hurry use_configuration
,this will lead to
mgm-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 troublesuser_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 🙂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. 🙂
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 yetmgm-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 theinstaller.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.
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.
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.
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.
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 aflagsmith
service for the front-end which is connected to thepostgres
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 thefreqtrade
service without any issues.Which wouldn't be an issue if we simply run both
mgm-hurry
andfreqtrade
in a single container service.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:
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 theuser_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.
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 withoutcommand +=
.A cleaner implementation would be:
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:
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)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:
mgm-hurry switch_configuration
--override
modifier (Default intention would be to simply switch between existing configs instead of overriding)--new
modifier:False
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 a
config-name
--config
,--config_ho
&--config_private
):config-name
(With or without.json
, append.json
if not provided by user), since all config files will exist inuser_data
.json
file existing inuser_data
+ additionalnew config file
option.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)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.
config-name
is passed with--new
in the command, automatically use (skip the corresponding interactive list prompt)--new
is passed ornew config file
was selected, check if config file already exists:config-name
config file with a new copy ofmgm-config.example
ormgm-config-private.example
if not existingmgm-config.example
ormgm-config-private.example
mgm_config_names
section of.hurry
Then we can make it so they only show the correct config file types in their selection, some examples:
mgm-config
files will be pre-fixed withmgm-config-
, example: user fills inusdt
, new file will be calledmgm-config-usdt.json
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
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
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.
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 singleif 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 theself.freqtrade_cli.installation_exists(silent=True)
andself.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 🙂
![mgm-hurry-silent](https://user-images.githubusercontent.com/24852597/146637749-0756ba34-6808-497a-9a0d-e91996d2d411.png)
For example in the
mgm-hurry up
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'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:
/usr/bin/env sh <(curl -s "https://raw.githubusercontent.com/Rikj000/MoniGoMani/development/installer.sh")
mgm-hurry install_freqtrade
&mgm-hurry install_mgm
(Now that I think more about this, perhaps we should rename the commands to
mgm-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.