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

Pythonic submodules architecture #198

Closed
wants to merge 1 commit into from
Closed

Pythonic submodules architecture #198

wants to merge 1 commit into from

Conversation

lrq3000
Copy link
Member

@lrq3000 lrq3000 commented Jul 18, 2016

Should fix #176, #245 and the second issue in #188 by reorganizing tqdm modules with a more pythonic architecture. This will allow a small performance boost (for all modules including core) as noted in #258 (comment).

The main goal of reorganizing tqdm's modules architecture is to avoid unnecessary imports without relying on delayed imports nor wrappers (which bring a whole lot of other issues, like no help message in IPython or other interpreters and the inability to call class methods such as tqdm_notebook.write()).

The minor goal is to take this reorganization opportunity to enhance the overall API (uniformization for example, but if you have other ideas/complaints, please shout out in the discussion!).

Basically, I implemented the architecture suggested by @wernight:

  • each module is now exposed publicly (eg, _tqdm_notebook.py -> notebook.py):
# Before:
from tqdm import tqdm_gui
# Now:
from tqdm.gui import tqdm
  • submodules import and declaration in __all__ were removed from __init__.py
  • all classes are now named tqdm() and trange(), this uniformizes the usage of tqdm across all modules. For example, it will ease the end-user implementation of adapting import codes.
    Before:
if ipython_notebook:
    from tqdm import tqdm_notebook as tqdm
    from tqdm import tnrange as trange
else:
    from tqdm import tqdm, trange

for element in tqdm(some_iterator):
    pass

Now:

if ipython_notebook:
    from tqdm.notebook import tqdm, trange
else:
    from tqdm.core import tqdm, trange

Note that I don't consider this change mandatory, but I wanted to profit of the opportunity that we are anyway changing the library architecture to see if API uniformization could be done. I am very open to feedback about this.

  • __init__.py still imports and expose tqdm.core as the default tqdm, so the old API for the core module is still compatible:
# Both are equivalent
from tqdm import tqdm, trange
from tqdm.core import tqdm, trange

This also implies that tqdm.core is always imported even if the user only uses a submodule such as tqdm.gui, but since anyway all current submodules subclass from tqdm.core, this doesn't change anything (and anyway importing tqdm.core is not really heavy since it doesn't rely on any third party module...).

Do you find this new architecture and API easier to use? Any feedback is welcome! (Users are also welcome to participate!)

TODO:

  • Update README
  • Write in CONTRIBUTION the smooth rolling scheme (and modules acceptance guidelines) as described in Proposal for smooth rolling releases #252.
  • Write list of submodules with their status in README.
  • Test examples/ scripts.

@coveralls
Copy link

coveralls commented Jul 18, 2016

Coverage Status

Coverage decreased (-2.4%) to 78.987% when pulling 9b7cf20 on new_architecture into bb53160 on master.

@lrq3000 lrq3000 changed the title Reorganize tqdm modules architecture to avoid unnecessary imports Pythonic submodules architecture for tqdm Jul 18, 2016
@lrq3000 lrq3000 changed the title Pythonic submodules architecture for tqdm Pythonic submodules architecture Jul 18, 2016
@lrq3000 lrq3000 added the help wanted 🙏 We need you (discussion or implementation) label Jul 18, 2016
@codecov-io
Copy link

codecov-io commented Jul 18, 2016

Codecov Report

❗ No coverage uploaded for pull request base (master@1104d07). Click here to learn what that means.
The diff coverage is 46.15%.

@@            Coverage Diff            @@
##             master     #198   +/-   ##
=========================================
  Coverage          ?   77.81%           
=========================================
  Files             ?        9           
  Lines             ?      622           
  Branches          ?      115           
=========================================
  Hits              ?      484           
  Misses            ?      137           
  Partials          ?        1

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1104d07...8c2c88f. Read the comment docs.

@lrq3000
Copy link
Member Author

lrq3000 commented Aug 5, 2016

A very interesting slides presentation about successful APIs such as request:

https://speakerdeck.com/kennethreitz/python-for-humans

We indeed keep the API as the most important objective of tqdm since it's clearly this "feature" that greatly helped in the currently widespread adoption. Then I guess performance of the core tqdm should be the second priority, then simple/elegant code (for maintenance), and then the rest.

All that is to say that if anybody has any idea on how to further enhance the tqdm API, please speak now!

@lrq3000
Copy link
Member Author

lrq3000 commented Aug 5, 2016

Talking about API, here's a maybe interesting module to generate documentation (but it seems like it's reinventing the wheel, can't see the difference with doxygen...):
https://github.com/BurntSushi/pdoc

@CrazyPython
Copy link
Contributor

@lrq3000

  • I strongly want tqdm to have some PEP 484 type stubs. They help your editor/linter out a lot.
  • We should seriously have a "Performance Fest" where we try to inch that 80ns to something more like 70ns.
  • We should have a tqdm_bare_bones with only bare bones features for speed.

@lrq3000
Copy link
Member Author

lrq3000 commented Aug 6, 2016

About tqdm_bare_bones, there is a stripped down version retaining the most important features of tqdm in the perf test:

def simple_progress(iterable=None, total=None, file=sys.stdout, desc='',

It's currently used only for the perf test to ensure that core tqdm does not get slower than 2 times the bare bone version, so tqdm is still pretty efficient given all the features that are currently implemented! So yeah, we can easily put it in a separate submodule if asked.

@lrq3000
Copy link
Member Author

lrq3000 commented Aug 6, 2016

Type hints are very cool, this brings pure Python closer to Cython and Julia, but it's unfortunately only available for Python >= 3.5.2 right now. Maybe it will be backported to Python 2 in the future (although MyPy already backported it)? Also, we would have to ponder whether the added verbosity (and thus code obfuscation) is worth the tradeoff (for the moment, typing is not really useful in Python but I guess it will be in the future for performance optimizers such as PyPy). Also, tqdm is made to be highly flexible (working with any iterable type etc.), so we probably won't be able to statically type everything.

@CrazyPython
Copy link
Contributor

CrazyPython commented Aug 6, 2016

@lrq3000 As I said, include stub files. You can provide *.pyi files of the same name as the executable files. Python 2 ignores stub files and it's part of the official standard.

Some issues you mentioned

  • PyPy has already stated they will ignore type hints for practice reasons.
  • Type hints aren't reinforced
  • tqdm accepts any iterable - just use typing.Iterable
  • Type Hints help with the reading of code in my opinion - and extra stub files won't obfuscate the code.

@lrq3000
Copy link
Member Author

lrq3000 commented Aug 7, 2016

Ah great! I didn't see that separate stub files were possible (I thought they were talking about stub like in the sense of unit tests). About PyPy and Cython not using type hints, yes I just saw that here :(

Ok great, so here is the provisional TODO list to implement type hinting in tqdm:

(moved to #260).

/EDIT: I need to moderate a bit my excitement because it seems the current Python 3.5.2 implementation of type hints is only provisional and meant to get evolved in 3.6/3.7, particularly for duck typing support. Also, request's author tried to implement stub files and he wasn't particularly convinced of the usefulness for really pythonic APIs such as what we aim for tqdm:

I suppose the TLDR of this email is that I think that libraries with 'pythonic' APIs are the least likely to take up this typing system because it will provide the least value to them. Maintaining signatures will be a pain (stub files are necessary), writing clear signatures will be a pain (see above), and writing signatures that are both sufficiently open to be back-compatible and sufficiently restrictive to be able to catch bugs seems like it might be very nearly impossible.

This change doesn't catch any of the bugs anyone was going to hit (no-one is passing a callable to this parameter), and it doesn't catch any of the bugs someone might actually hit (getting the tuple elements in the wrong order, for example). At this point the type signature is worse than useless.

So yeah, we can try to implement the stub files but we shouldn't expect too much out of them at least until 3.6, then we'll see if they get more useful.

@lrq3000
Copy link
Member Author

lrq3000 commented Aug 7, 2016

I tried to install and use mypy on Windows and on Python 2.7, and it's not working. I don't know if this is a bug in the current version, but there is no mention in the documentation that stubs are supported in Python 2, only comment-based syntax (see here and here).

So for the moment I'll drop this until it matures a bit more. Because I can't implement something that I cannot test.

@CrazyPython
Copy link
Contributor

@lrq3000 try on Unix? Are you using Cygwin? and get a $5 raspberry pi for testing (raspian preinstalled), python isn't that great with windows

@CrazyPython
Copy link
Contributor

CrazyPython commented Aug 9, 2016

@lrq3000 It may be a pain, but it really does help my editor a lot. Seriously, I type annotate every function I write, and I won't be using the code in a week.

It's already a stable feature, and don't worry about incompatibility: you can just delete the .pyi files if they don't work.

Do it! I'm tempted to open a PR just for this! One time I even wrote a wrapper just for the type annotations for a library function.

Try the PyCharm editor. It is really good at inferring types until you get to library functions. It's a hassle to deal with.

@lrq3000 lrq3000 added need-feedback 📢 We need your response (question) and removed help wanted 🙏 We need you (discussion or implementation) labels Sep 4, 2016
@lrq3000
Copy link
Member Author

lrq3000 commented Sep 6, 2016

Given the new insight about the performance impact of the current architecture of __init__.py (which would be alleviated by the new pythonic architecture), I would like to push this PR further and merge it ASAP @casperdcl you are OK with that. Retrocompatibility with core tqdm is assured, only the API for submodules will change.

Hopefully, I would like to push before the end of September this PR as v5.0 along with #250, #257 (and maybe #244 if OK for you Casper) and include new submodules in alpha stage as #248, #258. We should leave #251 (monitoring thread) and other PRs for later.

@lrq3000
Copy link
Member Author

lrq3000 commented Sep 7, 2016

About #258 maybe not now, in fact it's almost ready, we just have to choose whether we keep functional tqdm_bare or class-based tqdm_bare_class, and then just copy some unit tests. So if it can be done for v5.0 that would be great but if not ready we'll leave it for later. I think this PR is of utmost importance, we should not move forward with new submodules until we have merged this PR.

@CrazyPython
Copy link
Contributor

@lrq3000 this is quite old already

@lrq3000 lrq3000 mentioned this pull request Sep 7, 2016
4 tasks
@lrq3000
Copy link
Member Author

lrq3000 commented Sep 7, 2016

Yes but it's a major change to the API, it's normal that it takes time. But I don't want to go further without @casperdcl 's approval, I think we should both agree when we do any change to the API architecture.

@casperdcl
Copy link
Sponsor Member

@lrq3000
Copy link
Member Author

lrq3000 commented Sep 10, 2016

@casperdcl The opposite, #198 should be last (because else we will have to recode manually the other PRs I think). I can manually redo #198 once the others are merged.

@lrq3000
Copy link
Member Author

lrq3000 commented Sep 10, 2016

@casperdcl Except #198 that should be last, the order is correct.

This was referenced Oct 16, 2016
@casperdcl
Copy link
Sponsor Member

also TODO: make things like #482, #494 easier (direct access to variable dictionary rather than having to parse bar strings in a custom stream)

@casperdcl casperdcl mentioned this pull request Mar 19, 2018
@casperdcl
Copy link
Sponsor Member

casperdcl commented Jun 3, 2018

proposed submodule structure:

key
^ contains tqdm(class), trange()

additionally, for a transition period:

  • _tqdm_notebook
  • _tqdm_gui
  • _main
  • tqdm.pandas()

@chengs
Copy link
Contributor

chengs commented Sep 14, 2018

@casperdcl
perhaps tqdm.auto is better than tqdm.autonotebook

@casperdcl
Copy link
Sponsor Member

k's, will change it

@chengs
Copy link
Contributor

chengs commented Sep 14, 2018

Move the content of #610 here.

Since some critical errors are reported, which are all related to TMonitor, global thread, or global tqdm.__instances set.

Perhaps, it is better to provide a simple version (maybe tqdm.simple.tqdm and tqdm.simple.trange), without the intergration of the functions above. It will look like just a wrapper of an iterator.

class tqdmSimple():
    def __init___():
        self.iters = iters
        #only this, nothing else
        #no Tmonitor
        #no cls.__instances.

    def __iter__(self):
        for obj in self.iters:
            yield ....
            [print progress]

Indeed, it will not have some good features such as auto refresh, but it won't cause complicated errors). And then, tqdm with all features can be constructed on the basis of the simple version.

For instance, the simple_progress in the test cases.

@chengs
Copy link
Contributor

chengs commented Sep 15, 2018

@casperdcl maybe also consider #604 (nosetest to pytest for test cases)? And use py3 to run performance tests (as py2.7 is going to be outdated)?

casperdcl added a commit that referenced this pull request Sep 21, 2018
`auto` will be able to support alternative backends in future

- 97a9393#commitcomment-29850169
- 97a9393#commitcomment-30466004
- related to #198
@casperdcl casperdcl self-assigned this Apr 1, 2019
casperdcl added a commit that referenced this pull request Aug 26, 2019
@casperdcl casperdcl added this to Next Release in Casper Aug 30, 2019
Casper automation moved this from Next Release to Done Sep 17, 2019
@casperdcl casperdcl mentioned this pull request Sep 17, 2019
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
need-feedback 📢 We need your response (question)
Projects
Casper
  
Done
Development

Successfully merging this pull request may close these issues.

should not import optional dependencies until actually necessary
6 participants