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

pythonPackages.astroquery 0.3.10 -> 0.4 #80552

Merged
merged 1 commit into from Feb 29, 2020
Merged

pythonPackages.astroquery 0.3.10 -> 0.4 #80552

merged 1 commit into from Feb 29, 2020

Conversation

xbreak
Copy link
Contributor

@xbreak xbreak commented Feb 19, 2020 •
edited

Motivation for this change

ZHF: #80379

Things done
  • Added test-prerequisite pytest-astropy-header and pytest-filter-subpackage.
  • Made basic tests in nix-shell using this library.

Check phase is now OK.

  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@xbreak xbreak changed the title pythonPackages.Astroquery 0.3.10 -> 0.4.0 pythonPackages.astroquery 0.3.10 -> 0.4.0 Feb 19, 2020
@ofborg ofborg bot requested a review from smaret February 19, 2020 18:02
@xbreak xbreak changed the title pythonPackages.astroquery 0.3.10 -> 0.4.0 pythonPackages.astroquery 0.3.10 -> 0.4 Feb 19, 2020
jonringer
jonringer reviewed Feb 20, 2020
View reviewed changes
Copy link
Contributor

@jonringer jonringer left a comment

Choose a reason for hiding this comment

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

please checkout from github to get unit tests https://github.com/astropy/pytest-astropy-header/tree/master/tests, the sdist on pypi doesn't contain any:

running build_ext

----------------------------------------------------------------------
Ran 0 tests in 0.000s

OK
Finished executing setuptoolsCheckPhase

pkgs/development/python-modules/pytest-astropy-header/default.nix Outdated Show resolved Hide resolved
@xbreak
Copy link
Contributor Author

xbreak commented Feb 20, 2020 •
edited

please checkout from github to get unit tests https://github.com/astropy/pytest-astropy-header/tree/master/tests, the sdist on pypi doesn't contain any:

@jonringer: Done. Thanks.

smaret
smaret suggested changes Feb 20, 2020
View reviewed changes
pkgs/development/python-modules/astroquery/default.nix Outdated
};

propagatedBuildInputs = [ astropy requests keyring beautifulsoup4 html5lib ];
propagatedBuildInputs = [ astropy requests keyring beautifulsoup4 html5lib numpy ];
Copy link
Member

Choose a reason for hiding this comment

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

I don't think that numpy is needed here. astroquery depends on astropy and astropy itself depends on numpy.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, thanks.

Copy link
Contributor

Choose a reason for hiding this comment

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

if it's listed in install-requires, you'll want to keep it. astropy is free to drop a dependency in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's no install requires in setup.py. But numpy is imported in various places and their own CI seems to always install it (https://github.com/astropy/astroquery/blob/7fa89014d2743028f4263473b5b0343e0e6400a4/.travis.yml). Please advice.

pkgs/development/python-modules/astroquery/default.nix Outdated Show resolved Hide resolved
pkgs/development/python-modules/pytest-astropy-header/default.nix Outdated Show resolved Hide resolved
@xbreak xbreak force-pushed the astroquery branch 2 times, most recently from d409db0 to 4c2e900 Compare February 20, 2020 21:43
@ofborg ofborg bot requested review from costrouc, smaret and primeos February 20, 2020 21:45
@xbreak
Copy link
Contributor Author

xbreak commented Feb 20, 2020

So this sort of led down a rabbit hole of changes caused by new dependencies. Hopefully it looks ok now.

@jonringer
Copy link
Contributor

is the hypothesis bump really necessary? It essentially rebuilds all of python packges (it's a dependency of pytest, which is used heavily)

@xbreak
Copy link
Contributor Author

xbreak commented Feb 20, 2020

is the hypothesis bump really necessary? It essentially rebuilds all of python packges (it's a dependency of pytest, which is used heavily)

Depends if this is true: https://github.com/astropy/pytest-astropy/blob/master/setup.cfg#L41
I didn't try to patch it to accept an older version.

@jonringer
Copy link
Contributor

ah, this is awkward. Hmm....

@FRidh are you okay with a hypothesis_5 attr in the pythonPackages package set? similar to when we had pytest_4?

@FRidh
Copy link
Member

FRidh commented Feb 21, 2020

I think we should avoid it. One or a couple of packages is not worth it IMO

@xbreak
Copy link
Contributor Author

xbreak commented Feb 21, 2020 •
edited

Hypothesis dropped Python 2 support so a straight update isn't doable either.

@xbreak
Copy link
Contributor Author

xbreak commented Feb 22, 2020

I'm inclined to close this PR as the impact seems too big. But I made a last attempt on updating hypothesis for Python 3 only.

@jonringer
Copy link
Contributor

@xbreak, eventually the python community will migrate over to using the latest hypothesis. Eventually we will be adding hypothesis>=5.0 to nixpkgs, and at the time, this package would be easily merge-able. If you would like to wait a few weeks, we can pick this back up.

@xbreak
Copy link
Contributor Author

xbreak commented Feb 24, 2020

@jonringer: Sure, no problem. I'll leave the PR as-is for the moment then and rebase/update as necessary then?

@smaret
Copy link
Member

smaret commented Feb 24, 2020

Can we disable the tests for the time being? This would allow astroquery to be included in the next release. We can re-enable them once hypothesis>=5.0 has been added to nixpkgs.

@xbreak
pythonPackages.astroquery 0.3.10 -> 0.4
83ed077 
Tests are disabled until pytest-astropy is updated with
pytest-astropy-header.
@xbreak
Copy link
Contributor Author

xbreak commented Feb 25, 2020

@jonringer: I updated PR with only updated astroquery and disabled tests.

smaret
smaret approved these changes Feb 25, 2020
View reviewed changes
Copy link
Member

@smaret smaret left a comment

Choose a reason for hiding this comment

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

Tested on macos with nix-review. It looks good to me.

@smaret
Copy link
Member

smaret commented Feb 28, 2020

@jonringer Can you please merge this?

jonringer
jonringer approved these changes Feb 29, 2020
View reviewed changes
Copy link
Contributor

@jonringer jonringer left a comment

Choose a reason for hiding this comment

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

LGTM

I would like tests, but they can't be active due to python package set, so it's understandably not running right now

[4 built, 0.0 MiB DL]
https://github.com/NixOS/nixpkgs/pull/80552
2 package built:
python37Packages.astroquery python38Packages.astroquery

@jonringer
Copy link
Contributor

@GrahamcOfBorg build python37Packages.astroquery python38Packages.astroquery

@jonringer jonringer merged commit a5e82af into NixOS:master Feb 29, 2020
@smaret smaret mentioned this pull request Feb 29, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@smaret smaret smaret approved these changes

@jonringer jonringer jonringer approved these changes

@FRidh FRidh Awaiting requested review from FRidh

@costrouc costrouc Awaiting requested review from costrouc

@primeos primeos Awaiting requested review from primeos

Assignees
No one assigned
Labels
6.topic: python 8.has: clean-up 8.has: package (new) 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@xbreak @jonringer @FRidh @smaret