search

mperrin / poppy

Physical Optics Propagation in Python
BSD 3-Clause "New" or "Revised" License
177 stars 41 forks source link

Issues with astropy-helpers #170

Open mperrin opened 8 years ago

mperrin commented 8 years ago

splitting off this topic from https://github.com/mperrin/poppy/issues/160#issuecomment-225229716

Hi @eteq, @josePhoenix can give more of the details, but I think it's a bit of both.

For literally years, every so often I've encountered some random and hard-to-debug failures which eventually traced back to "the astropy helpers are doing something unexpected behind the scenes":

Each of the above has led to time-consuming debugging to track down what's going on, which has been a time sink and distraction from the main tasks we're actually trying to get done. I readily admit that software is hard, and we can't expect a bug-free existence or anything close to it. But overall given the limited functionality we're actually gaining from the astropy helpers (mostly some setup.py tweaks and the sphinx extensions), it seems like the overall return on time invested is negative.

josePhoenix commented 8 years ago

Additionally:

My experience has been that all the hacks and customizations in the helpers seem well-motivated by problems in Astropy, but our WebbPSF and POPPY projects don't have {as large a build matrix, C and Cython extensions, a desire to customize setup.py to the same extent}. So, the benefits of the package template and the helpers are negated by the additional complexity.

I think another problem is the lag time: when we run into an issue, it typically takes a day to identify the template or helpers as the underlying cause, then we open an issue, then another day or so passes until we get a resolution. This is a very good level of service for a volunteer project, but a very bad time constant when I'm blocked and can't make progress on something!

There's a nonzero probability that POPPY and WebbPSF will gradually accrete customizations of their own over time that end up being similarly opaque to contributors, but at least @mperrin or I will be able to explain what went into them 😄

josePhoenix commented 8 years ago

Something we inherited from the package template: this ConfigParser issue.

https://github.com/astropy/package-template/commit/c2aa884af9b32f223d440cf0c987026c0f6abb7e

eteq commented 8 years ago

@mperrin @josePhoenix - I understand your concern that it can be frustrating to chase down these issues on a code base you didn't write. Trust me, I know that pain very very well! And if it's really true that you are not now nor ever going to be gaining anything by using the template, then there's no reason you should use it.

That said, consider the possibility that this may be a confirmation bias problem. There are likely a number of things you've gotten "for free" because it just worked and you never saw that anything was wrong, and no one ever noticed. And on this bit:

My experience has been that all the hacks and customizations in the helpers seem well-motivated by problems in Astropy, but our WebbPSF and POPPY projects don't have {as large a build matrix, C and Cython extensions, a desire to customize setup.py to the same extent}. So, the benefits of the package template and the helpers are negated by the additional complexity. There's a nonzero probability that POPPY and WebbPSF will gradually accrete customizations of their own over time that end up being similarly opaque to contributors, but at least @mperrin or I will be able to explain what went into them

The problem lies in putting these paragraphs together. I totally believe that you don't need all of what's in the helpers. But if you do accrete a need for some of it in the future, and build your own version, I bet it will be incompatible. And more importantly, the wider community will not be able to work with you on it. So yes, you two will understand it better, but the 50 other people who could have contributed are now cut out, or have to make the choice of "do I learn the POPPY/WebbPSF way of handling packages, or the affiliated package template", which is a lose-lose for everyone. Especially given that the largest fraction of the time implementing this sort of stuff is always in working around the other bugs and documenting all the fiddly little details.

I think another problem is the lag time: when we run into an issue, it typically takes a day to identify the template or helpers as the underlying cause, then we open an issue, then another day or so passes until we get a resolution. This is a very good level of service for a volunteer project, but a very bad time constant when I'm blocked and can't make progress on something!

What I would hope for is that you help us make the helpers and package template better. Then you wouldn't be blocked: you could fix it right away and at the same time help the problem for others. Even if you think that means the best thing to do is throw it away and start over, then maybe that would be good for everyone! But someone has to do it.

Happy to chat more about in person on this topic if that makes more sense, given we're all in the same institution...

P.S.

Something we inherited from the package template: this ConfigParser issue.

I'm pretty sure that's actually due to changes in the Python interpreter (arguably a bug in that it was a subtle change that they probably thought wouldn't affect anything public but actually did). So that goes along with the point above that it's a fix "for free", it's just you happen to catch it in an intermediate state.

josePhoenix commented 8 years ago

I'm pretty sure that's actually due to changes in the Python interpreter (arguably a bug in that it was a subtle change that they probably thought wouldn't affect anything public but actually did). So that goes along with the point above that it's a fix "for free", it's just you happen to catch it in an intermediate state.

Well, yes, the change that broke that import is from Python. However, had we used the PyPA recommended template for a setup.py, we wouldn't have depended on that behavior at all for our setup.py to run.

Another issue is that the way we learned there were changes to pull from upstream was by having our build break and then going and looking around the affiliated package template for likely culprits, then looking around the repo for a fix to copy in manually.

We could have pulled in the changes via git, but that makes a really gnarly git history that mixes the package template development commits with our own development, making for vexing git bisect sessions. And we still wouldn't have known there was a change to pull in, unless we'd been watching for new issues on the package-template repo.

josePhoenix commented 8 years ago

I totally believe that you don't need all of what's in the helpers. But if you do accrete a need for some of it in the future, and build your own version, I bet it will be incompatible. And more importantly, the wider community will not be able to work with you on it. So yes, you two will understand it better, but the 50 other people who could have contributed are now cut out, or have to make the choice of "do I learn the POPPY/WebbPSF way of handling packages, or the affiliated package template", which is a lose-lose for everyone.

But the Astropy affiliated package template already imposes this choice! "Do I learn the Python way of packaging, or the Astropy way of packaging?" For example: in an Astropy project, you don't cd docs; make html, you python setup.py build_sphinx. You don't install pytest and py.test, you use a bundled pytest and a custom setup.py command. And a bunch of other little things that may surprise and alarm contributors (like monkeypatching in new global builtins?!).

What I would hope for is that you help us make the helpers and package template better. Then you wouldn't be blocked: you could fix it right away and at the same time help the problem for others.

I do contribute upstream when I can, but I don't feel like I can make effective contributions to the helpers/template. A lot of customizations are motivated by specific limitations in outside tools that someone on the Astropy project has a specific need to work around, but what those limitations are is hard to figure out (except by working backwards from the behavior observed, which I'm not usually doing until the behavior is abnormal...).

Happy to chat more about in person on this topic if that makes more sense, given we're all in the same institution...

Sure! We can set up a time. Marshall and I are somewhat occupied this week, and I believe he is going on vacation soon... so I thought I'd capture my thoughts in this issue.

josePhoenix commented 8 years ago

Another issue with the pytest customizations in Astropy:

josePhoenix commented 7 years ago

Astropy helpers and the package template strike again!

When building WebbPSF on ReadTheDocs, our latest version of POPPY (including the latest v2.0.1 astropy_helpers submodule) was unable to install:

Excerpt from https://readthedocs.org/projects/webbpsf/builds/5821195/:

[ snip ]
Searching for poppy>=0.5.0
Reading https://pypi.python.org/simple/poppy/
Downloading https://pypi.python.org/packages/22/39/6ced910b3355a7f7e764cb985b24dac8b9a46c215fb9c1461145896c57d5/poppy-0.6.0.tar.gz#md5=249065f2f7892077acb321d1bc646002
Best match: poppy 0.6.0
Processing poppy-0.6.0.tar.gz
Writing /tmp/easy_install-ha9cewkv/poppy-0.6.0/setup.cfg
Running poppy-0.6.0/setup.py -q bdist_egg --dist-dir /tmp/easy_install-ha9cewkv/poppy-0.6.0/egg-dist-tmp-nwihqd2i
/home/docs/checkouts/readthedocs.org/user_builds/webbpsf/envs/stable/lib/python3.5/site-packages/pkg_resources/__init__.py:188: RuntimeWarning: You have iterated over the result of pkg_resources.parse_version. This is a legacy behavior which is inconsistent with the new version class introduced in setuptools 8.0. In most cases, conversion to a tuple is unnecessary. For comparison of versions, sort the Version instances directly. If you have another use case requiring the tuple, please file a bug with the setuptools project describing that need.
  stacklevel=1,
Traceback (most recent call last):
  File "/home/docs/checkouts/readthedocs.org/user_builds/webbpsf/envs/stable/lib/python3.5/site-packages/setuptools/sandbox.py", line 157, in save_modules
    yield saved
  File "/home/docs/checkouts/readthedocs.org/user_builds/webbpsf/envs/stable/lib/python3.5/site-packages/setuptools/sandbox.py", line 198, in setup_context
    yield
  File "/home/docs/checkouts/readthedocs.org/user_builds/webbpsf/envs/stable/lib/python3.5/site-packages/setuptools/sandbox.py", line 248, in run_setup
    DirectorySandbox(setup_dir).run(runner)
  File "/home/docs/checkouts/readthedocs.org/user_builds/webbpsf/envs/stable/lib/python3.5/site-packages/setuptools/sandbox.py", line 278, in run
    return func()
  File "/home/docs/checkouts/readthedocs.org/user_builds/webbpsf/envs/stable/lib/python3.5/site-packages/setuptools/sandbox.py", line 246, in runner
    _execfile(setup_script, ns)
  File "/home/docs/checkouts/readthedocs.org/user_builds/webbpsf/envs/stable/lib/python3.5/site-packages/setuptools/sandbox.py", line 47, in _execfile
    exec(code, globals, locals)
  File "/tmp/easy_install-ha9cewkv/poppy-0.6.0/temp/easy_install-kmr20fds/astropy-helpers-0.4.8/setup.py", line 49, in <module>
    AUTHOR_EMAIL = metadata.get('author_email', '')
  File "/home/docs/.pyenv/versions/3.5.2/lib/python3.5/distutils/core.py", line 134, in setup
    ok = dist.parse_command_line()
  File "/home/docs/checkouts/readthedocs.org/user_builds/webbpsf/envs/stable/lib/python3.5/site-packages/setuptools/dist.py", line 347, in parse_command_line
    result = _Distribution.parse_command_line(self)
  File "/home/docs/.pyenv/versions/3.5.2/lib/python3.5/distutils/dist.py", line 472, in parse_command_line
    args = self._parse_command_opts(parser, args)
  File "/home/docs/checkouts/readthedocs.org/user_builds/webbpsf/envs/stable/lib/python3.5/site-packages/setuptools/dist.py", line 658, in _parse_command_opts
    nargs = _Distribution._parse_command_opts(self, parser, args)
  File "/home/docs/.pyenv/versions/3.5.2/lib/python3.5/distutils/dist.py", line 536, in _parse_command_opts
    "command class %s must subclass Command" % cmd_class)
distutils.errors.DistutilsClassError: command class <class 'setuptools.command.bdist_egg.bdist_egg'> must subclass Command

I saw something on the package template or helpers issue tracker that made me think ah_bootstrap.py is out of date. I went to astropy/package-template to find it has been reimagined as a Cookiecutter template. The file ah_bootstrap.py is no longer present.

Upon investigating the post-create hook in the Cookiecutter template, I see it's now in the helpers and only copied out on project creation.

So, I updated it in POPPY and ran the RTD build against POPPY master and that seemed to fix it.

eteq commented 7 years ago

@josePhoenix - so just to clarify: it worked after you did that, right?

(FWIW, the cookiecutter approach to the template is hopefully the first step to making the helpers more "a la carte")

josePhoenix commented 7 years ago

@mperrin can let you know. My favorite kind of bugs are the ones that only show up in production :)

josePhoenix commented 7 years ago

In any case, the problems with astropy-helpers have never been insurmountable. They've merely required a totally disproportionate amount of time and expertise in packaging internals to surmount.

eteq commented 7 years ago

totally disproportionate amount of time and expertise in packaging internals

Not to beat a dead horse... but my point is that this depends on what you think it should be proportionate to. Certainly there are "quick fixes" that would be faster (although probably not easier, so I think the packaging expertise is always necessary), but those come with other costs down the road... for example, if someone else has to pick up the maintenance (you know... hypothetically :wink:), it'll be easier if it's on shared infrastructure.

josePhoenix commented 7 years ago

There is a set of shared infrastructure for Python packages. Building an astronomy-specific overlay to attempt to save maintainers from having to understand this shared infrastructure means you carry the maintenance burden of all your dependencies AND of this astronomy-specific overlay.

If you're volunteering (or being deputized) to maintain WebbPSF, I wish you the best of luck. I think it's clear that this does not decrease the maintenance burden for casual contributors at this point, so perhaps it should be taken over by a non-casual contributor.

mperrin commented 7 years ago

Yeah. I think we can all agree that the overhead is more proportionate when that cost gets spread across multiple packages by the same maintainer -- which in fact @eteq and I were just discussing by email. :-)

This does raise the question of - for something like webbpsf which is not ever going to be merged into the main astropy trunk, as opposed to say photutils - to what extent the added overhead also adds value. Some of the customizations are very nice. But there's what feels like a recurring cost in having to pay attention to unpredictable changes in a package template which is a constantly moving target. When it works it's great - when it breaks, it's the last thing you want to end up having to deal with.

In any case as of 3 minutes ago, this is now my and @eteq's problem, and no longer @josePhoenix's. :-) Thanks again for all your many contributions over the last 3 years.

eteq commented 7 years ago

But there's what feels like a recurring cost in having to pay attention to unpredictable changes in a package template which is a constantly moving target. When it works it's great - when it breaks, it's the last thing you want to end up having to deal with.

Fair enough, @mperrin. The intent of this machinery is to have fewer problems than you'd have to deal with if you didn't use it. But I think a missing piece of this story is that we don't know about the problems you would have had if the helpers hadn't silently fixed things behind the scenes (e.g. there are constant CI versioning problems that are mostly transparently fixed by the machinery). But I honestly don't know what the balance is there for WebbPSF.

It may be that there's a fundamental tension in using the same machinery for more "advanced" cases (for which WebbPSF apparently is one), and the "basic" user who, without the template/helpers, wouldn't ever have managed to package or use tests or anything like that. The helpers currently serve this dual purpose (i.e., they are not just for things that are supposed to go into core astropy), and this may be a result of just that tension. The curious thing is that I would have thought WebbPSF is a fairly straightforward case. So it's a bit confusing why it seems to have a lot more problems than many of the affiliated packages or other template users...?

I should say, though: one good bit of feedback I've gotten from the conversations with @josePhoenix is that it would be useful if this machinery was more "a la carte". That is, you can not have pieces you don't need, which should lower the apparent burden on the maintainer (as they only get "what they ask for"). That's now a goal for the helpers, it just requires developer resources which of course are always stretched thin, especially for infrastructure.

Thanks again for all your many contributions over the last 3 years.

👍 🎆

mperrin commented 7 years ago

The curious thing is that I would have thought WebbPSF is a fairly straightforward case. So it's a bit confusing why it seems to have a lot more problems than many of the affiliated packages or other template users...?

Good question, and one I have wondered myself. One possibility is the WebbPSF codebase is something like 2 or 3 years older than Astropy itself, and thus long predates the helpers and package template. It wasn't originally created from the package-template repo in the nominal fashion, rather we pulled that code into the existing webbpsf repo and retrofitted it, more or less. Back in 2014 or so. Is it possible that has left some longer-term jagged edges where things don't work as smoothly as they should? In the particular case we encountered today, I think the problem was that pulling in the latest updates to the astropy-helpers submodule didn't also pull in the update to ah_bootstrap.py. Is that not the behavior that is seen for other package template users?

pllim commented 7 years ago

I didn't read everything here but am a bit shocked that your Travis CI matrix against astropy dev did not catch this earlier.