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.

Sign up for GitHub

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

Jump to bottom

Interactive jsonschema demo running on notebooks.ai #538

Merged
merged 9 commits into from
Apr 28, 2019
Merged

Interactive jsonschema demo running on notebooks.ai #538

merged 9 commits into from
Apr 28, 2019

Conversation

martinzugnoni
Copy link
Contributor

Hi @Julian, following our discussion here #506, this PR adds a link to the DEMO Notebook running on https://notebooks.ai/.

** Note that the link won't work until this PR is merged, because it looks for a DEMO.ipynb file which is still not present in Julian's repo. If you want to see how it will look like, you can take a look at it here

Anything here can be changed. Post your thoughts and we can iterate from there.

@codecov
Copy link

codecov bot commented Mar 6, 2019

Codecov Report

Merging #538 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #538    /-   ##
=======================================
  Coverage   96.24%   96.24%           
=======================================
  Files          19       19           
  Lines        2398     2398           
  Branches      305      305           
=======================================
  Hits         2308     2308           
  Misses         77       77           
  Partials       13       13

1 similar comment
@codecov
Copy link

codecov bot commented Mar 6, 2019

Codecov Report

Merging #538 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #538    /-   ##
=======================================
  Coverage   96.24%   96.24%           
=======================================
  Files          19       19           
  Lines        2398     2398           
  Branches      305      305           
=======================================
  Hits         2308     2308           
  Misses         77       77           
  Partials       13       13

Julian
Julian reviewed Mar 8, 2019
View reviewed changes
demo.yml
@@ -0,0 1,3 @@
requirements:
Copy link
Member

Choose a reason for hiding this comment

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

Does this file get magically used somehow? I don't see anywhere here using it.

If so, can we update it for jsonschema 3? I think everything above looks like it shouldn't require any changes to do so.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The demo.yml file is automatically used by notebooks.ai while loading the Docker container. It's basically loading a base container Image and then installing all Python packages under "requirements".

I updated jsonschema to version 3.0.1.

Copy link
Member

Choose a reason for hiding this comment

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

Got it -- and is it supposed to be the equivalent of a requirements.txt then?

Because you have pyrsistent here, but that's not the only dependency (or transitive dependency) of jsonschema.

Copy link
Member

Choose a reason for hiding this comment

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

I also don't really see any docs linked anywhere on https://notebooks.ai -- where do they keep those?

Copy link
Member

Choose a reason for hiding this comment

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

Hey @martinzugnoni! Polite ping on the above, I know life has gotten in the way for both of us :) but yeah curious to hear what you think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey @Julian, I'm very sorry about the late response. I've been super busy lately.

Requirements listed in the demo.yml file are the libraries needed to run the DEMO.ipynb notebook. It seems that just by installing jsonschema the DEMO is not fully working (see screenshot below). That's why I also added pyrsistent.

image

About notebooks.ai documentation, we are still working on it. We will probably link the documentation to the main website once it's done.

@Julian
Copy link
Member

Julian commented Apr 19, 2019 via email

@martinzugnoni
Copy link
Contributor Author

I've just checked and it's running pip install package1 package2 ... packageN with all the packages listed in the demo.yml file.

In this case, it would run:
pip install jsonschema==2.6.0 pyrsistent==0.14.7

@Julian
Copy link
Member

Julian commented Apr 24, 2019

Really odd...

I could of course just merge and make believe I didn't see this, but it worries me about future dependencies and whether that's going to mean now keeping them in both places -- I'm fairly sure jsonschema is doing the right thing here, but it's hard to tell what notebooks.ai is doing. It might be it's using a really old version of pip and/or setuptools?

But like, installing jsonschema installs pyrsistent as it should:

⊙  virtualenv venv; venv/bin/pip install jsonschema; venv/bin/python -c 'from jsonschema import validate'                                                              julian@Air
New pypy executable in /Users/julian/Development/jsonschema/venv/bin/python
Also creating executable in /Users/julian/Development/jsonschema/venv/bin/pypy
Installing setuptools, pip, wheel...
done.
DEPRECATION: A future version of pip will drop support for Python 2.7.
Collecting jsonschema
/Users/julian/Development/jsonschema/venv/site-packages/pip/_vendor/msgpack/fallback.py:222: PendingDeprecationWarning: encoding is deprecated, Use raw=False instead.
  PendingDeprecationWarning)
  Using cached https://files.pythonhosted.org/packages/aa/69/df679dfbdd051568b53c38ec8152a3ab6bc533434fc7ed11ab034bf5e82f/jsonschema-3.0.1-py2.py3-none-any.whl
Collecting attrs>=17.4.0 (from jsonschema)
  Using cached https://files.pythonhosted.org/packages/23/96/d828354fa2dbdf216eaa7b7de0db692f12c234f7ef888cc14980ef40d1d2/attrs-19.1.0-py2.py3-none-any.whl
Collecting pyrsistent>=0.14.0 (from jsonschema)
Requirement already satisfied: setuptools in ./venv/site-packages (from jsonschema) (41.0.1)
Collecting six>=1.11.0 (from jsonschema)
  Using cached https://files.pythonhosted.org/packages/73/fb/00a976f728d0d1fecfe898238ce23f502a721c0ac0ecfedb80e0d88c64e9/six-1.12.0-py2.py3-none-any.whl
Collecting functools32; python_version < "3" (from jsonschema)
Installing collected packages: attrs, six, pyrsistent, functools32, jsonschema
Successfully installed attrs-19.1.0 functools32-3.2.3.post2 jsonschema-3.0.1 pyrsistent-0.14.11 six-1.12.0

Will have a think.

@martinzugnoni
Copy link
Contributor Author

I agree @Julian. Don't worry, I will further investigate before we merge this. Will come back when I have any news. Thanks for reporting BTW.

@Julian
Copy link
Member

Julian commented Apr 24, 2019

Awesome! Thanks! Let me know what you find.

@martinzugnoni
Copy link
Contributor Author

WTF! Now I can't reproduce the error I pasted above :/

I just removed pyrsistent from the demo.yml file and it works fine (as expected). Could you please double check it and confirm me it's working also for you?

@Julian
Copy link
Member

Julian commented Apr 24, 2019

Hah, well, glad it's working now at least.

Does look like it's working here too!

@Julian
Copy link
Member

Julian commented Apr 24, 2019

The last thing from me I think at this point is really just the styling or size of that button :D -- is there anything quick that could be done about that? If not, happy to merge I think at this point -- much appreciated of course!

@martinzugnoni
Copy link
Contributor Author

Thank @Julian

That blue button is the default image we use everywhere. Feel free to propose a different button image if you want. I'm 100% open to any design change.

In case this button is at least acceptable, we can move forward and merge it for now, until we work in a redesign.

@Julian
Copy link
Member

Julian commented Apr 25, 2019

@martinzugnoni thanks for being so amenable!

I think what rubs my design feathers a bit is probably just having such a big button on its own line, it's even bigger than the text introducing it.

I think even just shrinking it by ~30% size-wise would look better visually, and even better would be possibly putting it underneath a small screenshot of the notebook? But even just the shrinking I think would be awesome.

And really thanks again regardless, looks like a great project.

@martinzugnoni
Copy link
Contributor Author

Cool @Julian, appreciate your comments. What about now?

Note that the notebook preview is a gif, which I think makes more sense to give the user a quick overview of how it will look like.

@Julian
Copy link
Member

Julian commented Apr 26, 2019

I think the gif is returning a 404 -- might need to have that alongside directly here in the repo or something to make sure it stays up? getting close

@martinzugnoni
Copy link
Contributor Author

Fixed it, the previous link expired. Now I hosted on Github.

@Julian
Copy link
Member

Julian commented Apr 28, 2019

Great, thanks!

@Julian Julian merged commit a723320 into python-jsonschema:master Apr 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Julian Julian Julian left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@martinzugnoni @Julian