askmike / gekko

A bitcoin trading bot written in node - https://gekko.wizb.it/
MIT License
10.07k stars 3.95k forks source link

[TODO] Simpler configuration #956

Closed cassvail closed 6 years ago

cassvail commented 7 years ago

It took some time to trying to understand the project configurations, which is poorly documented (I understand this takes time) I also had some troubles configuring the app in a "production" server, due to the many configurations all around the project

A few improvements could be implemented:

It's a bit of info, but I hope it helps!

askmike commented 7 years ago

Hey!

Great points, and you are 100% right. Let me quickly explain how we got to the current mess:

TLDR:


So what would you think is ideal? I don't think I want to fully remove single config files just yet (and use the TOML config files for configuring CLI gekko).

askmike commented 7 years ago

usually the backend who serves the UI is it's own API backend so relative urls would be enough. In case the backed is actually a different URL it could be the backend itself to serve the UI that information (moving again the configurationt to the backend config.js) At this point the UIconfig.js is empty :)

You are right about the backend, but the frontend needs to know where the backend is. That's the whole reason for that file: so the frontend can figure out how to contact the backend.

askmike commented 7 years ago

but I think it would be a lot simpler for both the cli and the backend in general to have a unique centralized configuration in the config.js (with the config-sample.js committed)

Two problems with this:

cassvail commented 7 years ago

Thanks for your explanation! I understand the points you explained, so I'll try to put my proposal under another perspective more Users focused, so you can tell me your opinion Also I want you to know that I'm aware of lack of knowledge on the implementation to understand how the whole framework is implemented

As I understand right now the config.js contains most of the configuration needed by both the backend and the UI. I would logically split the configurations in 2 types:

Now, let's say I download the application and I know all configuration are in a sample-config.js The sample-config contains all the default settings (localhost, SQLite, default EMA settings etc), which are the same for the UI and the CLI environment and it also contains the "run" settings, which services can be by default enabled or disabled (as it is right now)

I think in this scenario both the simple usability (as you stated user requested) and simple configuration are taken care of. There's nothing to configure by default

A couple of points on the UI design:

I tried to explain as best as I could, hope this helps ;)

thegamecat commented 7 years ago

I think the config should be in one place but I don't like the comments about the api server port etc. That should all be in the main config, but it should be easy to change the port in the config.

TBH, the UI is way too restrictive to be worth using imo and is now forcing me to manage settings in the config file and a Tomlinson, which is pointless. Indeed the overhead for the front end to function is not worth it. I tried it again earlier and it's still calling the api twice with large datasets, it's graphing of indicators isn't great and I just don't see how you can learn from it. Being noob friendly seems fine so long as it's actually possible for people to install, use the native indicators and make money. But is that actually the case?

askmike commented 7 years ago

Quick notes before I go into it:

if not it can still use/create a replica of the sample config (I think it's the current behaviour)

No, Gekko will crash with an error that you didn't specify a config. Editing the sample-config is a bad idea since it makes updating Gekko really hard (merge conflicts inside sample-config).

If a user wants to run the APIs on a different server a route localhost:3000/getApiUrl could be implemented and used by the UI (but I think is a very rare case)

This only works if the host is specified somewhere, since most people runnig Gekko in the cloud don't connect the UI directly to the gekko server but route traffic through nginx. And if it is specified somewhere we can also give it to the frontend :) (calling localhost:3000/getApiUrl is hard if that's not where Gekko is running).

Another reason for the UIConfig is so that if Gekko runs on the same machine as your browser Gekko will open the browser for you, pointing to the UI (as configured in the UIConfig).

if it doesn't find any i could generate the basic toml files using the config.js info (using ES6 or a templating library it's a very trivial job)

Great! As long as it works in node6 without transpiling.

So if I get you right, your main point is:

I would logically split the configurations in 2 types:

  • [defaults/core services]
  • [specific "run" settings (add to & overwrite the first type)]

Where the first part also houses stuff like DB settings and such. These two parts are used for both CLI and UI (where applicable).

This sounds perfect, though I am worried about the following use cas:


No need for the UI to know to much about the implementation of the core services, if it needs something the backend can serve the info to the UI or the UI can ask through routes. This means that the backend could be running with or without the UI, as it seems to be right now (but here I don't know how it was implemented the architecture, let's say that this separation of concerns is the basic feature provided by Single Page Application as of those built with vue.js)

I won't go into it too deep, but I want to abstract the config file from the user (when using the UI), but definitely not from the rest API: this should be a simple API wrapper around starting and managing gekkos. And the configs provide a lot of flexibility, why move away from this?

Maybe it is better to limit this issue to user facing configuration. API communication between the rest server and UI is a bit out of scope for this issue imo.

askmike commented 7 years ago

Do you have a proposal in mind for a (dummy / pseudo) file structure, and how it would work from the user when running over the CLI (what does the user need to edit when)?

askmike commented 7 years ago

An easier option would be:

Wouldn't this solve a lot of confusion?

cassvail commented 7 years ago

make it explicit in the docs that the config.json should be fully ignored UNLESS you are running over the CLI. And also moving the sample-config.json away from the root of the repo so that only users looking at CLI docs would ever find it

I agree with this! I think the sample-config.js can stay on the root a UI user should just "npm start" the project if thats the default in the docs

move UIConfig to root of repo and merge it with the baseConfig (which now can be fully ignored - since you can set everything in the UIConfig).

It would be correct to merge baseConfig with the config.js (alias sample-config.js). Have UIConfig.js only to define UI concerning settings like API configs. Moving it to the root is a big step forward 👍

Editing the sample-config is a bad idea since it makes updating Gekko really hard (merge conflicts inside sample-config)

In a UI startup, if the user has not created the config.js, the server could copy config-sample.js and use it as default config, the user MUST not edit the config-sample.js

No, Gekko will crash with an error that you didn't specify a config

I don't think there's need for the server to crash, reading the config is a script and errors could be handled to copy and use the copy of the sample-config.js instead (probably not for the CLI as you said)

Right now they can just use a single config file per instance, it should not be much harder in this new proposal.

In fact, the config.js (alias sample-config.js) remains almost the same as it is right now. If you have many "run configs", just put them in a array or some way to define mutliple ones the same config.js (probably as it is right now)

Great! As long as it works in node6 without transpiling.

http://es6-features.org/#StringInterpolation

Do you have a proposal in mind for a (dummy / pseudo) file structure, and how it would work from the user when running over the CLI (what does the user need to edit when)?

Recap of what just said: We would only need two files "committed" in the root: sample-config.js and UIconfig.js

Do you think this solves the different scenarios?

askmike commented 7 years ago

http://es6-features.org/#StringInterpolation

I am not sure how StringInterpolation can convert JSON into TOML dynamically? If there is a lib we can def use it though :)


if a user has created a config.js the backend will use that and start normally (toml file will be created if needed)

Yes this is always needed, since if users update the config.js we need to make sure these changes are applied to the UI.

In that case we need some heavy comments in all the toml files saying that you should NEVER edit these files manually, since they will always be overwritten on start.

But in that case why write the TOML files to disk at all? Just render them on the fly in the API and feed into the frontend?

A remote UI user, would have to just configure the UIconfig.js

Yes great! I would like to keep these configs apart, since it highlights that a config.js (or sample-config.js) do not have a host/port option. This makes it very clear that if you run node gekko (without UI) your machine will not start listening on some ports (big security impact).

cassvail commented 7 years ago

I am not sure how StringInterpolation can convert JSON into TOML dynamically?

For what I've seen TOML are just some ${key} = ${value} mappings. But I don't know if there's more to it...

But in that case why write the TOML files to disk at all? Just render them on the fly in the API and feed into the frontend?

Actually I don't know how TOML files are used, I just assumed that you needed them to be saved for some purpose, maybe to keep configs upon a restart. I don' know :)

This makes it very clear that if you run node gekko (without UI) your machine will not start listening on some ports

Keeping in mind that if a user wants to connect a remote UI, he will need that socket open. That could be still enable/disable setting in the config.js (nearby the port config in which the CLI would open the socket for remote or local UI connections)

askmike commented 7 years ago

For what I've seen TOML are just some ${key} = ${value} mappings. But I don't know if there's more to it...

A little bit more, but not too much (nested props, arrays). You are right it should not be too hard.

Actually I don't know how TOML files are used

Ah, they are only given to the frontend so that it can render config blocks (a proper solution would be a dynamically generated form based on the JSON - this means we can ditch TOML all together). Since they are only used for that we don't even need to write them to disk. So what I propose (95% the same as your proposal):

(easiest - stick to TOML for UI config)

(more work - ditch TOML)


Would that be acceptable? Main thing we would need (for easiest solution above) is a lib / module that can turn JSON into TOML, the rest is just glueing.


Keeping in mind that if a user wants to connect a remote UI, he will need that socket open.

To be clear:

So only in the second situation a config with any host/port is used (the UIConfig) - the first option ignores this and does not open ports.

cassvail commented 7 years ago

A little bit more, but not too much (nested props, arrays)

The question might be are this extented features actually in use or they could be simplified? I don't really know what actually users do

(easiest - stick to TOML for UI config) keep UIConfig. if no config file is present copy over sample-config.js to config.js. This has everything except UIConfig stuff (host / port / ssl). We feed TOML to the UI (via the rest API) which is dynamically generated based on the config.js file. The UI builds a dynamic config and gives this back to the API (already present for 90%).

Seems a very good solution

(more work - ditch TOML)

I would open another issue for this

To be clear: When I say CLI I specifically mean running a single gekko (backtest / import or live gekko) based on ONE config. A CLI Gekko will NEVER open a port on your machine (with the only exception if you use a plugin like the slack plugin which needs to listen for postCallbacks). When I say UI I mean starting a gekko (over the CLI) with --ui, yes you still start and stop it with the CLI, but it starts a server and does not start any Gekko by itself (unless instructed via REST api).

Yeah, this kinks of explains better how things are going under the hood

askmike commented 7 years ago

The question might be are this extented features actually in use or they could be simplified?

Nested is used a lot, basically all nested objects (more than 1 layer deep) are nested inside the TOML (example - everything under simulationBalance is nested inside the key simulationBalance). So when rendering JSON into TOML order is of great importance (else you are rendering nested props).

Okay great! We have our work cut out for us. Would you be willing to work on this? Else I would put it on the end of my TODO.

cassvail commented 7 years ago

I see if I can find some time to get to work on something in the weekend, maybe starting to move and fix the config file paths. I'm very new to gekko so about testing the whole application feature might be difficult to me, but I think you could work that out Meanwhile i found this lib https://github.com/kenany/json2toml which could save time on toml conversion

askmike commented 7 years ago

Awesome thanks! That lib looks great, we could use that straight away (if it works).

I think the easiest start would be creating a config.js file yourself and updating the configPart route to read in the config.js, take a section (example config.tradingAdvisor) and turning that into TOML using the lib you linked to.

After that you can fully remove the /config folder including all the TOML. That way nothing changes for the frontend but we have dynamic config parts (all coming from the "main" config.js).

Lbatson commented 7 years ago

Is there any reason why the UI has to use TOML? Could it just be JSON instead? JSON syntax is going to be more familiar for anyone using it. I'd never even heard of TOML before (and still really unfamiliar how to deal with nesting, arrays, etc). I'd be glad to help work on converting it over because I'm running into an issue where running my strategy in the CLI now requires a separate TOML config file now to run it in the UI. Mainly I want to see the buy and sells plotted out and it's inconvenient they're separate (unless they aren't and I'm missing something).

askmike commented 6 years ago

@Lbatson I missed your reply apologies:

Is there any reason why the UI has to use TOML? Could it just be JSON instead? JSON syntax is going to be more familiar for anyone using it.

The main goal of the UI is to allow for non technical users to use Gekko. I know we are not quite there (you need to manually install gekko's dependencies via NPM, start gekko via CLI), but I want to move towards that goal.

People who are very familiar with JSON (it's actually a hard syntax for non tech: forget one , and you're doomed with vague technical messages about "parsing errors").

I'd never even heard of TOML before (and still really unfamiliar how to deal with nesting, arrays, etc).

In the direction I am going I want to separate users from producers, only the latter need to worry about how to map a TOML file to some struct. The first can just "fill in the TOML".

2 possible solutions to what you describe:

askmike commented 6 years ago

I'm afraid this won't be coming anytime soon :(

In the new UI will tackle server configuration (host, ports, auth, etc) through a wizard, not via a config file anymore. Also will probably factor out TOML files.