Skip to content

Plotly Offline #236

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

Merged
merged 28 commits into from
Jul 3, 2015
Merged

Plotly Offline #236

merged 28 commits into from
Jul 3, 2015

Conversation

chriddyp
Copy link
Member

No description provided.

res = requests.get(download_url + '/sourcefiles.json')
res.raise_for_status()

with open(os.path.join(plotlyjs_path, 'sourcefiles.json'), 'w') as f:
Copy link
Contributor

Choose a reason for hiding this comment

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

what are these sourcefiles.json ?

Copy link
Member Author

Choose a reason for hiding this comment

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

they contain the list of plotly.js dependencies:

{
    "files": ["dependencies/d3.v3.min.js", "dependencies/jquery-latest.js", "dependencies/typedarray.js", "plotly.min.js"]
}

that way, if we change the dependencies in the future we don't have to change anything on the python lib side. they just explain which files are necessary to include and to download

Copy link
Contributor

Choose a reason for hiding this comment

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

LOVE IT

Copy link
Contributor

Choose a reason for hiding this comment

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

could this be a bundle we build in streambed and then a single download file for folks?

Copy link
Member Author

Choose a reason for hiding this comment

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

👍

@etpinard
Copy link
Contributor

This is the best thing that happen to IPython since IPython notebook.

@chriddyp
Copy link
Member Author

please try it out and review @aneda @cldougl @theengineear ! thx!

@chriddyp
Copy link
Member Author

@BRONSOLO @theengineear - did you guys change the folders endpoint on prod? this branch and master are failing in the test_duplicate_folders function.

@theengineear
Copy link
Contributor

@chriddyp , it's a minor break, looks like 409 400 for the particular issue. I just checked out api tests, the 400 is what we're currently expecting. Mind if we change this to a TestCase while we're in here : D ?:

"""
test_meta:
==========

A module intended for use with Nose.

"""
import random
import string
from unittest import TestCase

import plotly.plotly as py
from plotly.exceptions import PlotlyRequestError


class FolderAPITestCase(TestCase):

    def setUp(self):
        py.sign_in('PythonTest', '9v9f20pext')

    def _random_filename(self):
        random_chars = [random.choice(string.ascii_uppercase)
                        for _ in range(5)]
        unique_filename = 'Valid Folder'+''.join(random_chars)
        return unique_filename

    def test_create_folder(self):
        try:
            py.file_ops.mkdirs(self._random_filename())
        except PlotlyRequestError as e:
            self.fail('Expected this *not* to fail! Status: {}'
                      .format(e.status_code))

    def test_create_nested_folders(self):
        first_folder = self._random_filename()
        nested_folder = '{0}/{1}'.format(first_folder, self._random_filename())
        try:
            py.file_ops.mkdirs(nested_folder)
        except PlotlyRequestError as e:
            self.fail('Expected this *not* to fail! Status: {}'
                      .format(e.status_code))

    def test_duplicate_folders(self):
        first_folder = self._random_filename()
        py.file_ops.mkdirs(first_folder)
        try:
            py.file_ops.mkdirs(first_folder)
        except PlotlyRequestError as e:
            self.assertTrue(400 <= e.status_code < 500)
        else:
            self.fail('Expected this to fail!')

@theengineear
Copy link
Contributor

This is the only reference to a 409 i can find:

https://github.com/plotly/python-api/blob/master/plotly/plotly/plotly.py#L774

I don't think the library is breaking because of this test :)

Also, I tested the above test changes in python 2.7, i don't think i used TestCase stuff that wasn't around in 2.6... but maybe.

import requests


def download_plotlyjs(download_url, plotlyjs_dir='~/.plotly/plotlyjs'):
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we get rid of the download_url param and just create a streambed url that we know and can authenticate at?

Then, we can just write a function to decide whether the requesting user has access?

Additionally, why do we need the plotlyjs_dir arg? is there a good reason for folks to need to change this? Also, i think it's not used in the func?

(obviously, the download_url is perfect for the current dev though :))

Copy link
Contributor

Choose a reason for hiding this comment

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

Finally, if this didn't require any arguments, we could just call it the first time a user tries to use it, which would mean they don't need to know about it.

Copy link
Member Author

Choose a reason for hiding this comment

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

great idea, going to punt 👟 🏈 this one to a future release

@theengineear
Copy link
Contributor

@chriddyp , playing around with this now, nice work!

@theengineear
Copy link
Contributor

I noticed that when you open notebooks back up, nothing seems to run until the plot is drawn:

image

@theengineear
Copy link
Contributor

that said, super cool that opening them backup just works

image

@theengineear
Copy link
Contributor

nice! plot loads up in nbviewer, that's super solid!

http://nbviewer.ipython.org/gist/theengineear/3b986f62836453d470ea

@chriddyp
Copy link
Member Author

  • bundle it up
  • test in old ipython notebook
  • investigate moving plotting calls to be async to make the notebook load quick
    the empty notebook is waiting for the huge notebook to be served
    image
  • test on windows
    image
  • configure links for enterprise
    image
  • better warning indicator if trying to call iplot before calling init_notebook_mode

v2

  • compress json
  • let download_plotlyjs authenticate and download without url

@chriddyp
Copy link
Member Author

@etpinard - due to some conflicts in ipython 2.4.1, I'm going to bundle up plotly.js w/out jquery 1.8.3 and instead rely on ipython's 2.0.3 version of jquery.

here's the test suite on the different versions:

at first glance, versions look and interact the same

@chriddyp
Copy link
Member Author

@etpinard try zooming on the 2dhistogram_contour_subplots example.. do you see the black background? (happens on all versions)

@chriddyp
Copy link
Member Author

@theengineear nice tinkerbell! throwback!

@theengineear
Copy link
Contributor

Not opposed to adding ipython to core-tests, but, then we need to add it as a requirement in setup.py.

I'd suggest we go with ipython[all] or at least ipython[notebook].

I just did a quick search for ipython tests and it seems that there's some support out there for testing in a notebook: https://pypi.python.org/pypi/pytest-ipynb

I haven't really tried we might be able to test everything we need to outside of a notebook environment. Again, just haven't really tried.

@chriddyp
Copy link
Member Author

chriddyp commented Jul 1, 2015

yeah, nvm, I thought that the test_optional wasn't being run by circle... but it is! @theengineear - can I get a 💃 ?


def download_plotlyjs(download_url):
if not os.path.exists(plotlyjs_path):
os.makedirs(plotlyjs_path)
Copy link
Contributor

Choose a reason for hiding this comment

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

can you check for write permissions and gracefully exit here? I think the tools.py module defines it:
https://github.com/plotly/python-api/blob/master/plotly/tools.py#L61-76

_file_permissions

Copy link
Member Author

Choose a reason for hiding this comment

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

👍

@theengineear
Copy link
Contributor

non-blocking, are we pushing this up to pypi though? i feel like we might want to just allow folks to download from github master for a bit?

👯

@chriddyp
Copy link
Member Author

chriddyp commented Jul 2, 2015

@theengineear - yeah, I'm gonna push it up to pypi as those openssl errors that bubbled up through the git+ install scares me.

@theengineear
Copy link
Contributor

thx @chriddyp !

@azarezade
Copy link

Sorry if I ask trivial question
but how can I setup plotly with offline mode available?

chriddyp added a commit that referenced this pull request Jul 3, 2015
@chriddyp chriddyp merged commit 2598650 into master Jul 3, 2015
@nicolaskruchten nicolaskruchten deleted the offline branch June 19, 2020 16:09
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.

4 participants