Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Devops #54

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Devops #54

wants to merge 4 commits into from

Conversation

barrysteyn
Copy link
Contributor

@barrysteyn barrysteyn commented Nov 20, 2017

  1. Removed the devops folder - not only is packer an overkill, but it did not work
  2. Dockerfile, working with python 3.6 in alpine container

The Dockerfile works, and demonstrates how a command will be run. CMD is just coin list, however the CMD docker command (unlike Entrrypoint) is meant to be overridden. So it can therefore run whatever is wanted

requirements.txt Outdated
requests==2.18.4
six==1.11.0
SQLAlchemy==1.1.14
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm trying to keep the requirements minimal so I made SQLAlchemy an optional extra in the setup.py file.

Do you know which packages require autobahn and txaio?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can ignore autobahn (that was picked up by mistake). Autobahn is the WAMP implementation for Python. WAMP is in turn a weird WS protocol that Poloniex used in their v1 version (which does not work anymore) of their WS feed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

However, SQLAchemy is needed. If I don't install it, and try run it inside the container, it complains!

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but it's an optional dependency. You install it with:

pip install numismatic[SQL]

It's already specified in the setup.py file in the extras_require directive.

I want to change the SQL to sql actually. There's a visualisation piece I want to add which will require pandas and I'll put that change in at the same time.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two things:

  1. I am not familiar with optional dependencies, but if it is optional, then it should work without it needing to be installed. This is not the case now. So it's not optional (in my mind)
  2. Installing pandas will require the container be much more complex than it already is. I am against the visualization for two reasons: (a) It will complicate the container (b) I think visualization should be an "add-on" module, and not part of numismatic cli

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is optional in that the FileCollector works without sqlalchemy or Pandas. If you want to use the SqlCollector then you have to install the sql dependencies. If you want to use the DataframeCollector you'll have to install pandas.

Copy link
Owner

@snth snth Nov 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With regards to the container, you can have a minimal one to run the collector in the background. You could provide another one with more optional extras installed or leave that for people to do themselves.

I do think that visualisations make a big difference, especially when it comes to selling stuff.

Dockerfile Outdated
COPY ./ /usr/src/app
RUN pip install -e .
WORKDIR numismatic
CMD coin listen -f poloniex -a btc collect run
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very happy to have a minimal Dockerfile.

I think the CMD will need to be something more generic though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I put that command there for a demo. The minimalism is all due to you removing Pandas... Pandas needed some very hectic dependencies in order for it to work...

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think the Dockerfile is the place to have a demo. I'll submit a commit with a suggestion once I can try it on a Linux box.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I agree. When I get back home, on the 28th, I will change it to work better...

@snth
Copy link
Owner

snth commented Nov 20, 2017

Do we need to remove the devops folder? I haven't looked at it but I liked the bash aliases which I thought might be useful.

@barrysteyn
Copy link
Contributor Author

Regarding the Devops folder, we can keep some things, but it will take a bit of work. In fact, using packer is such a huge overkill, that I deemed it not really worth it...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants