Fix modules in anticipation of future testing updates by drdavella · Pull Request #6450 · astropy/astropy

drdavella · 2017-08-16T21:11:05Z

In anticipation of eventually being able to invoke tests directly from pytest (and other testing updates), it is necessary to fix a few quirks in the way that packages are structured and modules are loaded.

Like #6449, these changes are based on work from #6437. Even if that PR is not integrated as-is, this is a necessary update.

This manifests itself during the test collection phase of pytest (when invoked directly from the command line). It seems like during test collection 'locals' gets polluted and so it is not a reliable way to load the parameters into the module's namespace.

astropy-bot · 2017-08-16T21:11:06Z

Hi there @drdavella 👋 - thanks for the pull request! I'm just a friendly 🤖 that checks for issues related to the changelog and making sure that this pull request is milestoned and labelled correctly. This is mainly intended for the maintainers, so if you are not a maintainer you can ignore this, and a maintainer will let you know if any action is required on your part 😃.

Everything looks good from my point of view! 👍

astrofrog · 2017-08-17T07:37:56Z

Thanks! I propose we wait until we have made a decision about invoking tests from pytest before moving ahead with this.

pllim · 2017-08-17T13:37:47Z

IMHO the cosmology change is not strictly related to pytest but rather some namespace magic that affects pytest. I think the change proposed here is cleaner and easier to maintain regardless of other pytest changes.

pllim · 2017-08-17T13:41:40Z

CircleCI failure is very cryptic for me:

docker pull astropy/astropy-32bit-test-env:1.9

Pulling repository docker.io/astropy/astropy-32bit-test-env
Tag 1.9 not found in repository docker.io/astropy/astropy-32bit-test-env

docker pull astropy/astropy-32bit-test-env:1.9 returned exit code 1

Action failed: docker pull astropy/astropy-32bit-test-env:1.9

drdavella · 2017-08-17T13:41:55Z

@astrofrog, @pllim I actually think the configobj.py should be deleted regardless of whether we support pytest directly because it's completely redundant and ignored in almost all cases. I feel less strongly about the other change except as it relates to pytest.

drdavella · 2017-08-17T13:44:19Z

@pllim maybe restart and 🙏 for success? The travis failure appeared to be unrelated but I see it's already been restarted.

pllim · 2017-08-17T13:45:12Z

Re: CircleCI -- I don't have admin access to that but I think @astrofrog does.

drdavella · 2017-08-17T13:46:24Z

Btw, I'm in the process of creating an issue that is sort of a testing manifesto, so hopefully it can be used to spur some discussion in advance of the coordination meeting (which hopefully I'll be able to attend).

mhvk · 2017-08-17T14:49:24Z

I think these changes are good independent of anything related to pytest, and would suggest just merging them. My only question is whether a changelog entry is even necessary.

mhvk · 2017-08-17T14:50:29Z

p.s. @drdavella - thanks for looking at this in detail - I actually quite like the idea of having tests run directly with pytest, and, independently, very much like the cleanup of the testing code you are doing.

larrybradley · 2017-08-17T15:23:18Z

CircleCI was a transient failure (downloading the docker image). I restarted it and all passes now.

pllim · 2017-08-17T15:24:01Z

Re: Change log -- I agree that it is not necessary. The change is not something typical user would notice or care about.

drdavella · 2017-08-17T15:38:10Z

I just reverted the change log (oops forgot [ci skip], sorry). I can squash if you want.

pllim · 2017-08-17T15:45:45Z

Let's wait to squash until this PR is approved. For future reference, you can also do a git rebase -i HEAD~n and just drop the unwanted commit.

drdavella · 2017-08-17T17:22:50Z

Travis failed again :(

mhvk · 2017-08-18T15:01:21Z

 )

-available = tuple(k for k in locals() if not k.startswith('_'))
+# If new parameters are added, this list must be updated


Looking at this again, why not keep the autogeneration, but just ensure it works? E.g.,

available = [k for k in locals() if isinstance(k, dict) and 'reference' in k]

(I'd also remove the explicit addition from core.__all__ and instead write there __all__ = [...] + parameters.available or add them in the loop further down, but that is a bit beyond this PR...)

This seems reasonable but I worry that it might be a little unreliable. What is the chance of randomly having another dict in your namespace with a member named reference? Maybe it's not huge but it doesn't seem impossible.

It almost seems like each of these should be an instance of a new class like cosmology.Parameter or something like that (which would inherit dict), and then the test would be as simple as:

available = [k for k in locals() if isinstance(k, Parameter)]

But maybe that's too much of a change?

If one had a class, it would be quite trivial to have it have a registry as well. But probably overkill, and then it gets even further away from the primary purpose of this PR... The logic for my suggestion was that it keeps the code more similar but still allows for not having to delete imports.

Also, in either case, since locals() returns a dictionary, it will need to look like the following:

all_locals = locals() available = [k for k in all_locals if isinstance(all_locals[k], WhateverClass)]

At the risk of being called stubborn, I updated the auto-generation with a simple class-based scheme. It seems pretty clean and introduced a minimal and largely invisible change, but I can do it the other way if that seems more reasonable.

mhvk · 2017-08-18T15:02:32Z

While dealing with my remaining comment, maybe rebase and get rid of the two changelog commits?
And this PR is obvious enough that I'll merge once that is done and tests pass.

drdavella · 2017-08-18T16:54:39Z

-# __all__ in core.py
+# in addition to the list above.
+
+class Parameter(dict):


If we keep this, maybe it should be called Cosmology. Doh.

drdavella · 2017-08-18T17:10:28Z

Current failures are real. I didn't test against Py2 before pushing. Working on a fix.

drdavella · 2017-08-18T18:43:55Z

I'm not sure what to make of the repeated travis failures. It seems to be the same test every time. I really don't see how any of these changes could have that effect, but I don't know. Is it possible that a particular travis VM is just flaky? Would a particular VM instance be bound to the same test case every time?

mhvk · 2017-08-18T18:56:03Z

Exactly the same problem is happening for other PRs, so it is unrelated (@bsipocz?).

I'd much prefer not to introduce a new class in this PR. At least I won't merge it without asking for the cosmology maintainer's input, and it seems quite possible we'll end up bikeshedding. But up to you.

pllim · 2017-08-18T19:25:52Z

Re: Travis failure -- Doesn't look related. Probably flaky connection to astropy data server at that time.

drdavella · 2017-08-18T19:37:36Z

To be honest, I feel like using a class is the 'right' way to accomplish this, because it guarantees that you get what you ask for. It seems Pythonic. The other approach is a heuristic that could backfire, however small that probability is, and could lead in the future to bug that would be pretty hard to diagnose. But maybe I'm just paranoid.

However, as you say, we've spent enough time bikeshedding, and I don't want to be any more of a hard head than I have already. My vote at the moment is just to revert to using an explicit list of cosmologies but update __all__ in core.py to automatically populate the parameters. Listing the parameters explicitly isn't so bad because it's what every other module does with __all__ anyway.

mhvk · 2017-08-18T20:00:06Z

Sounds good. Am all in favour of not needlessly updating multiple lists with the same information!

mhvk · 2017-08-19T17:13:54Z

OK, this looks all OK now, so I'll merge. Thanks, @drdavella! I'm also looking forward to the rest of the pytest simplifications - right now what we have only works on pytest 3.1! (#6418)

Clean-up modules, partially in anticipation of future testing simplifications.

drdavella added 2 commits August 16, 2017 16:58

Remove redundant file for configobj extern package

2c9be68

pllim added cosmology external PRs and issues related to external packages vendored with Astropy (astropy.extern) testing labels Aug 17, 2017

pllim requested review from astrofrog and bsipocz August 17, 2017 13:38

pllim added this to the v2.0.2 milestone Aug 17, 2017

larrybradley added the no-changelog-entry-needed label Aug 17, 2017

drdavella mentioned this pull request Aug 17, 2017

Long-term testing improvements and future directions #6451

Closed

mhvk reviewed Aug 18, 2017

View reviewed changes

drdavella force-pushed the fix-packages branch from 5285f58 to d52f5f6 Compare August 18, 2017 16:50

drdavella commented Aug 18, 2017

View reviewed changes

Automatically update cosmology core namespace from parameters

071e714

drdavella force-pushed the fix-packages branch from 2549faa to 071e714 Compare August 18, 2017 20:58

mhvk merged commit 88fc374 into astropy:master Aug 19, 2017

drdavella deleted the fix-packages branch August 21, 2017 13:03

bsipocz pushed a commit that referenced this pull request Sep 6, 2017

Merge pull request #6450 from drdavella/fix-packages

4ddcadb

Clean-up modules, partially in anticipation of future testing simplifications.

Conversation

drdavella commented Aug 16, 2017

Uh oh!

astropy-bot Bot commented Aug 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

astrofrog commented Aug 17, 2017

Uh oh!

pllim commented Aug 17, 2017

Uh oh!

pllim commented Aug 17, 2017

Uh oh!

drdavella commented Aug 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

drdavella commented Aug 17, 2017

Uh oh!

pllim commented Aug 17, 2017

Uh oh!

drdavella commented Aug 17, 2017

Uh oh!

mhvk commented Aug 17, 2017

Uh oh!

mhvk commented Aug 17, 2017

Uh oh!

larrybradley commented Aug 17, 2017

Uh oh!

pllim commented Aug 17, 2017

Uh oh!

drdavella commented Aug 17, 2017

Uh oh!

pllim commented Aug 17, 2017

Uh oh!

drdavella commented Aug 17, 2017

Uh oh!

mhvk Aug 18, 2017

Choose a reason for hiding this comment

Uh oh!

drdavella Aug 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhvk Aug 18, 2017

Choose a reason for hiding this comment

Uh oh!

drdavella Aug 18, 2017

Choose a reason for hiding this comment

Uh oh!

drdavella Aug 18, 2017

Choose a reason for hiding this comment

Uh oh!

mhvk commented Aug 18, 2017

Uh oh!

drdavella Aug 18, 2017

Choose a reason for hiding this comment

Uh oh!

drdavella commented Aug 18, 2017

Uh oh!

drdavella commented Aug 18, 2017

Uh oh!

mhvk commented Aug 18, 2017

Uh oh!

pllim commented Aug 18, 2017

Uh oh!

drdavella commented Aug 18, 2017

Uh oh!

mhvk commented Aug 18, 2017

Uh oh!

mhvk commented Aug 19, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

astropy-bot Bot commented Aug 16, 2017 •

edited

Loading

drdavella commented Aug 17, 2017 •

edited

Loading

drdavella Aug 18, 2017 •

edited

Loading