eyeonus / Trade-Dangerous

Mozilla Public License 2.0
97 stars 31 forks source link

Dockerization #80

Closed jonathan-irvin closed 3 years ago

jonathan-irvin commented 3 years ago

Super basic install for docker.

eyeonus commented 3 years ago

No.

jonathan-irvin commented 3 years ago

Wow, ok. I was willing to handle all of this for you, just needed some input. This isn't meant to replace the workflow that you already have, only to make it more compatible with multiple environments.

Maybe the threading part was a little off-color, but you'd be surprised how flexible this is and how easy it would be to setup. Python can be a little tricky, but adding this into a container allows you to control the version of python that's used (line 1 of my change).

But just saying "No" is a little harsh, don't you think? The checkboxes were more for me than anything else.

Anyway, back when I did trading, I found a lot of use in your tool and wanted to contribute to your project.

Thanks for taking the time to look at it!

eyeonus commented 3 years ago

I'm not willing to complicate this already extremely complicated thing I inherited. I see absolutely no benefit to doing this.

As far as multi-threading is concerned, whether this thing was dockerized or not, it would need a complete overhaul on the back-end in order to even make it possible to do it.

No, I don't think saying "No" is harsh.

eyeonus commented 3 years ago

As far as wanting to contribute, you're more than welcome to submit code, ideas, what-have-you. I even have an issue up specifically directed at those who want to help.

That doesn't mean I'm going to agree with your ideas or put them into TD.

I literally spent 5 HOURS yesterday fixing the current deployment. Replacement or not, accepting this would mean more work for me down the line, regardless of who does the work of initial implementation.

I barely have any time to work on this as is, and I'm not willing to add more maintenance work to my load.

jonathan-irvin commented 3 years ago

Fair enough. This sort of thing is my specialty and it would require very little on your part now and in the future. It would enhance your build process and make it tremendously easier for others to contribute as well as consume your inherited, awesome tool.

I'll continue this on my fork and figure it out on my own. I seemed to have hit a nerve. It's a great tool and it's made me a pretty penny before.

I'm only pitching this because of Python, really. It's a little finicky on Macs, but wrapping it in docker gets me the environment I need without all of the documentation fuss, but to each their own. Have a happy holiday!

o7

eyeonus commented 3 years ago

You have, thus far, made exactly one claim that I find plausibly true, that being that it make it easier for users to use TD, but I am not convinced that it would be "tremendously" easier. The process currently, for a standard install, is to install Python>=3.4, open a console/terminal, and type "pip install trade-dangerous". That's it. Any dependencies are handled by pip during installation, and the only time anyone has ever had problems with installing/running TD -at least problems enough to inform me of them, anyway- is when they are doing something funky, such as running it via venv.

As for the rest of your arguments, you have either failed to provide any supporting evidence for them at all, or they are really just not good arguments in the first place, such as being able to lock TD to a specified version of Python.

Exactly how would this "enhance [the] build process"? This is written in Python, which is a run-time interpreted language. It doesn't get built.

How exactly would 'dockerization' "make it tremendously easier for others to contribute"? Is Docker some kind of magical programming language that anyone and their mom can understand and easily write code in? No. It's a virtual environment container. That's it. TD would still be written in Python, contributions would still need to be written in Python, all that would really happen is that you would be adding ANOTHER dependency on top of all the existing requirements.

And that, my friend, is the key issue- dockerization means having to install something NOT Python in order to run a Python program.

If you want to fork this and make a docker-TD, that's your business. TD has an open-source license, so by all means, go ahead.

If you want to make an external program that uses TD as its back-end like the .NET-based TDHelper does, you're welcome to. No one's stopping you.

But I will never approve a PR that causes TD to require anything other than Python itself to run.

Valien commented 3 years ago

How exactly would 'dockerization' "make it tremendously easier for others to contribute"? Is Docker some kind of magical programming language that anyone and their mom can understand and easily write code in? No. It's a virtual environment container. That's it. TD would still be written in Python, contributions would still need to be written in Python, all that would really happen is that you would be adding ANOTHER dependency on top of all the existing requirements.

And that, my friend, is the key issue- dockerization means having to install something NOT Python in order to run a Python program.

Docker, in this branded/generic sense, but any OCI compliant container is just a sandboxed environment with an immutable and lightweight OS that utilizes your native desktop kernel for underlying processing. It can be used for virtual environments and development (and production) hence why it's has grown in popularity. In some ways you can imagine a Python venv as something akin to it (except a container is immutable and read-only).

You wouldn't be adding any dependency on top of TD at all. Users can easily run TD as normal but if there was a Dockerfile included as @jonathan-irvin was trying to summit in a PR all that does is provide a user with an option to run it that way if they want.

eyeonus commented 3 years ago

"It's popular" is not an effective argument.

Mansplaining to me does not help your case.

As I've already said, I see no benefit to adding this to TD.

At best, I would consider adding a "how to install with Docker" page on the wiki that includes the dockerfile on it.

alexdresko commented 3 years ago

Docker is a much lighter, much more powerful, much less annoying dependency than Python ever could be. Docker support is such a simple thing that brings so much potential.

jonathan-irvin commented 3 years ago

You wouldn't be adding any dependency on top of TD at all. Users can easily run TD as normal but if there was a Dockerfile included as @jonathan-irvin was trying to summit in a PR all that does is provide a user with an option to run it that way if they want.

This was my point. It changes nothing with your process, just makes it easier for people to consume the app in a different way.

Docker works similar to TravisCI where it creates a thin environment that your app runs on that is consistent and optionally persistent (with bind mounts).

I know Python is your only main dependency, but it can be a pain just to get Python going! Anyway, you've made your point not to consume this PR and that's 100% your prerogative. If you're fine with agreeing to disagree, I'm cool with working on it on my own fork. I may hit you up with some questions about what folder paths are used so I can properly mount the volumes and pull in the data.

Again, not trying to start a fight or anything, but let me answer your questions the best I can:

Exactly how would this "enhance [the] build process"? This is written in Python, which is a run-time interpreted language. It doesn't get built.

Adding a Dockerfile standardizes the environment that your app runs on similar to how TravisCI tests your app (it probably uses Docker already). So, this enhances the build process if you have a ton of dependencies by including them already in the container and locking the versions so you have a common environment to work off of or test.

How exactly would 'dockerization' "make it tremendously easier for others to contribute"?

Again, standardizing the environment needed between host machines allows you to isolate variables in the deployment process. So, no matter what version of Python is installed on the host machine, this ensures that everyone is using the same version. There's less room for errors and troubleshooting if everyone is working off of a common environment.

dockerization means having to install something NOT Python in order to run a Python program.

Not really. It's 100% optional. 9 times out of 10, once you make a Dockerfile you rarely have to mess with it. If people want to run it in a container, they can do so without affecting your workflow at all.

I noticed that you use Semantic Release (which is awesome, I love it too). One of the options you can do is to tag docker images. I know you publish to pip, but you can also tag docker images in the same way. Very easy to troubleshoot if you can choose a specific version of your tool (which already comes bundled with a specific version of Python).


Anyway, I wasn't looking to shake things up or touch any nerves. Assume good intent. I was trying to help because I was having issues with getting Python going on my mac (and didn't want to even bother with it on Windows), but Docker is a neat tool where I can just say "start with that one version of Python, install the app, and run a command to pre-populate the databases".

One thing I'd like to also do is get the GUI running from the container, but I may hit you up with some questions later...if you're cool with that.

eyeonus commented 3 years ago

Docker is a much lighter, much more powerful, much less annoying dependency than Python ever could be. Docker support is such a simple thing that brings so much potential.

Docker isn't a programming language. Using Docker would not have any effect on that, because TD is still a PYTHON program.

jonathan-irvin commented 3 years ago

Docker is a much lighter, much more powerful, much less annoying dependency than Python ever could be. Docker support is such a simple thing that brings so much potential.

Docker isn't a programming language. Using Docker would not have any effect on that, because TD is still a PYTHON program.

Yep! It's a tool. Nothing more.

eyeonus commented 3 years ago

I know Python is your only main dependency....

Python isn't a dependency. It's the language TD is written in.

Exactly how would this "enhance [the] build process"? This is written in Python, which is a run-time interpreted language. It doesn't get built.

Adding a Dockerfile standardizes the environment that your app runs on similar to how TravisCI tests your app (it probably uses Docker already). So, this enhances the build process if you have a ton of dependencies by including them already in the container and locking the versions so you have a common environment to work off of or test.

TD has two dependencies that are not part of the standard Python installation: 'requests' and 'AppJar'. Neither of those dependencies is version dependent.

How exactly would 'dockerization' "make it tremendously easier for others to contribute"?

Again, standardizing the environment needed between host machines allows you to isolate variables in the deployment process. So, no matter what version of Python is installed on the host machine, this ensures that everyone is using the same version. There's less room for errors and troubleshooting if everyone is working off of a common environment.

Sorry, no. That explains how it could make it easier to detect errors and to troubleshoot, but none of the reasons given have anything to do with how easy it is to contribute to TD.

Contributing to TD means contributing code. What version of Python is being used by the coder only matters in case of backwards incompatibilities, which is an extremely rare thing in the case of Python: last time it happened, it got split into Python2 and Python3. TD does not work in Py2.

As far as contributing is concerned, all that matters is- does the code you have added to TD work as intended? There are no environmental variables that matter in the slightest, unless of course the contribution involves using environmental variables.

Finding problems is not contributing. Fixing them is, and fixing something written in Python requires knowledge of Python. That's it.

dockerization means having to install something NOT Python in order to run a Python program.

Not really. It's 100% optional. 9 times out of 10, once you make a Dockerfile you rarely have to mess with it. If people want to run it in a container, they can do so without affecting your workflow at all.

That sounds like a really good argument for putting a Docker install how-to on the wiki, but not at all for putting a dockerfile in TD's code.

I noticed that you use Semantic Release (which is awesome, I love it too). One of the options you can do is to tag docker images. I know you publish to pip, but you can also tag docker images in the same way. Very easy to troubleshoot if you can choose a specific version of your tool (which already comes bundled with a specific version of Python).

That sounds exactly like the kind of 'more work' I don't want.

Anyway, I wasn't looking to shake things up or touch any nerves. Assume good intent. I was trying to help because I was having issues with getting Python going on my mac (and didn't want to even bother with it on Windows), but Docker is a neat tool where I can just say "start with that one version of Python, install the app, and run a command to pre-populate the databases".

Mac installs aren't supported because none of us (the contributors) have a Mac. If you want to update the Mac install page on the wiki, please feel free, it could use the love. https://github.com/eyeonus/Trade-Dangerous/wiki/Setup-Guide#macusers

One thing I'd like to also do is get the GUI running from the container, but I may hit you up with some questions later...if you're cool with that.

I am perfectly fine with it. I'm not objecting to you making a dockerfile for TD, I'm not saying you shouldn't do it or that I hate you for trying. I'm saying that it doesn't belong in TD's code. Possibly the wiki, but definitely not the code.