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

#126 : Support for result storage to a MySQL database #127

Merged
merged 9 commits into from
Nov 3, 2016

Conversation

tangi75
Copy link
Contributor

@tangi75 tangi75 commented Nov 2, 2016

Here are the changes to support battery generation for a web server and result storage (in JSON) to a MySQL database.

@vsoch
Copy link
Member

vsoch commented Nov 2, 2016

Excellent work! I should be able to review this later this morning.

@tangi75
Copy link
Contributor Author

tangi75 commented Nov 2, 2016

Thank you Vanessa!

@vsoch
Copy link
Member

vsoch commented Nov 2, 2016

ok want to give some quick feedback on what I'm working on - I think (for development and deployment without dependencies) it would be great to have a docker-compose.yml to set up a mysql database and web server, given that the user doesn't want to do that. In my case, I don't want to configure a sql database on my local machine (or elsewhere) to have to test, haha.

@vsoch
Copy link
Member

vsoch commented Nov 2, 2016

And I'm going to reorganize the folder structure a bit so the user can (in the future) select a Dockerfile for a different database, for example expfactory-python/server/sql and expfactory-python/server/postgres and the compose example can go in the folder with the matching database (sql for now) and if you add support for others, can go in there too :) Going to work on this for a while - back in bit!

@tangi75
Copy link
Contributor Author

tangi75 commented Nov 2, 2016

Excellent!

@tangi75
Copy link
Contributor Author

tangi75 commented Nov 2, 2016

Let me know if I can help.
One remark: expfactory-python/server/mysql, rather than expfactory-python/server/sql, would be more appropriate I think.

@vsoch
Copy link
Member

vsoch commented Nov 2, 2016

Hit a snag - it just needs to be set up to work with python 3, so that needs to be done first.

@tangi75
Copy link
Contributor Author

tangi75 commented Nov 2, 2016

Argh! I left the port to Python 3 on hold. It works fine with me but I'm not using the expfactory command nor have I looked at your unit testing suite to make sure the coverage is good enough to manage the risk of breaking things on Python 2.

Here are the commands I ran to partially port to Python 3:
cd "python -c 'import site; print(site.getsitepackages()[0])'/expfactory/"
find . -name "*.py" | xargs sed -i -e 's/iteritems/items/g'
find . -name "*.py" | xargs sed -i -e 's/print \(.*\)$/print(\1)/'
find . -name "*.py" | xargs sed -i -e 's/SimpleHTTPServer/http.server/'
find . -name "*.py" | xargs sed -i -e 's/SocketServer/socketserver/'

sed -i -e 's/from exceptions import ValueError//' survey.py
sed -i -e '322s/^./\ \ \ \ \ \ \ \ /' survey.py

sed -i -e 's/urllib2/urllib.request/' utils.py
sed -i -e 's/__init__/expfactory.__init__/' utils.py
sed -i -e '84s/"rb"/"r"/' utils.py
sed -i -e 's/basestring/str/' utils.py

sed -i -e 's/ \([^ ]*\.keys()\)\(\[.*\]\)/ list(\1)\2/' battery.py

But I noticed at least one remaining problem in expfactory/vm.py:
for v in range(len(default_inits[deployment])): jspsych_var = default_inits[deployment].keys()**[v]** jspsych_val = "\n".join([str(x) for x in default_inits[deployment].values()**[v]**])

What snag did you hit precisely? I may go on the Python 3 port but I'm not familiar with P2 vs P3 incompatibilities so I may need some time...

@vsoch
Copy link
Member

vsoch commented Nov 2, 2016

Yeah we are going to have issues with items vs. iteritems, it's just one big headache really, which is why I've been avoiding it :)

@tangi75
Copy link
Contributor Author

tangi75 commented Nov 2, 2016

OK, just step back and let me try to make the code compatible with both Python 2 and 3 on another branch...

@vsoch
Copy link
Member

vsoch commented Nov 2, 2016

No don't worry about this for now - I'm installing anaconda2 so I can also use python 2 - the python 2-->3 is a much larger issue we can deal with for another separate thing.

@tangi75
Copy link
Contributor Author

tangi75 commented Nov 2, 2016

As you wish

@tangi75
Copy link
Contributor Author

tangi75 commented Nov 2, 2016

I created this Pull Request without my Python 3 patches but I confess I have not tested it on Python 2.

@tangi75
Copy link
Contributor Author

tangi75 commented Nov 2, 2016

I just ran find . -name "*.py" | xargs 2to3 and I have to admit you're right, I underestimated the level of effort :)

@vsoch
Copy link
Member

vsoch commented Nov 2, 2016

Ok this isn't actually working for me, but here is something you can work from:

https://github.com/vsoch/expfactory-python/tree/storage_in_a_database

I probably can't work on this for a bout - I spent all day on it at the detriment to my two jobs, which is probably a bad thing to do :) Keep me updated here and I'll look when I get the chance, thanks! Ideally, here is what we want to do:

  • update this code so that the generation with the Dockerized mysql works (right now the index.html is just empty, I don't see errors in the console, no time to debug
  • include all parameters that need to go into the template (eg the webserver base) as options to specify in the script to generate it
  • test and be sure that this works for one and one + experiments

Thanks!

@tangi75
Copy link
Contributor Author

tangi75 commented Nov 2, 2016

OK, I take over. Thank you Vanessa for your irresponsible efforts :)

@vsoch
Copy link
Member

vsoch commented Nov 3, 2016

Haha, you're awesome :) I'll definitely be able to jump back in, just need to be responsible first!

Keep me updated!

@vsoch
Copy link
Member

vsoch commented Nov 3, 2016

@tangi75 take a look - I think it looks good! When tests pass, and it looks good to you, I'm ready to merge! We could arguably add tests or the like, but I think this is a really great start, and it's important to get the additional feature out there for use (and then we can update as necessary!)

@vsoch vsoch merged commit c4fe469 into master Nov 3, 2016
@vsoch vsoch deleted the storage_in_a_database branch November 3, 2016 21:44
@tangi75
Copy link
Contributor Author

tangi75 commented Nov 3, 2016

I'm out with no laptop on hands but thanks to the magic of timezones, I'll have reviewed everything when you wake up tomorrow.
Have a great day!

@tangi75
Copy link
Contributor Author

tangi75 commented Nov 4, 2016

Everything works fine :)
I created a tiny PL to avoid inserting blank data

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