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

python3Packages.aioinflux: init at 0.9.0 #70840

Closed
wants to merge 2 commits into from

Conversation

liamdiprose
Copy link
Contributor

@liamdiprose liamdiprose commented Oct 9, 2019

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • Ubuntu 18.04
    • 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 nix-review --run "nix-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.

@jonringer
Copy link
Contributor

this has your other PR in it, I would just close the other one, and keep this one.

Also, can you please squash your commits so that they just related to the two packages, commit history should look like:

pythonPackages.ciso8601: init at <ver>
pythonPackages.aioinflux: init at 0.9.0

@liamdiprose liamdiprose force-pushed the aioinflux-0.9.0 branch 4 times, most recently from 03ee334 to e748b0f Compare October 9, 2019 22:22
@liamdiprose
Copy link
Contributor Author

liamdiprose commented Oct 9, 2019

Thanks for the pointers; I've squashed the commits for the two packages.

Hopefully everything works with rewritten history 😥

Do you still want me to rebase everything on origin/master?

@jonringer
Copy link
Contributor

as long as there's no merge conflicts, rebasing isn't necessary

@liamdiprose
Copy link
Contributor Author

I thought that would be the case, thought I'd just check anyway. I only learnt how to rebase 2 hours ago 😛

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.

nix-review passes on NixOS
diff LGTM

[6 built, 10 copied (176.9 MiB), 31.3 MiB DL]
https://github.com/NixOS/nixpkgs/pull/70840
3 package were build:
python27Packages.ciso8601 python37Packages.aioinflux python37Packages.ciso8601

pkgs/development/python-modules/aioinflux/default.nix Outdated Show resolved Hide resolved
@jonringer
Copy link
Contributor

@GrahamcOfBorg build python27Packages.ciso8601 python37Packages.aioinflux python37Packages.ciso8601

@liamdiprose liamdiprose force-pushed the aioinflux-0.9.0 branch 2 times, most recently from 3787dc2 to 85df678 Compare October 10, 2019 20:37
@jonringer
Copy link
Contributor

@GrahamcOfBorg build python27Packages.ciso8601 python37Packages.aioinflux python37Packages.ciso8601

@risicle
Copy link
Contributor

risicle commented Oct 10, 2019

Two things:

  1. The tests for aioinflux don't actually run if you inspect the logs. I suspect their setup.py wasn't actually hooked up to a pytest test-runner. But when you set up a custom checkPhase to make them actually run you realize that they almost all use local networking and so won't work in the sandbox. So you may as well disable the tests and remove all those deps. 🤷‍♂️

  2. You're pointing at the wrong github repo for aioinflux. It's actually at https://github.com/gusutabopb/aioinflux.

@jonringer
Copy link
Contributor

good catches @risicle :)

@liamdiprose
Copy link
Contributor Author

Very good catches indeed. I did notice the 0 tests ran, but have no idea why I thought nothing of it.

doCheck is now false, and the test dependencies have been removed.

Out of curiosity, would it be possible to do the network tests in the check stage? Something like the bittorrent test.

@risicle
Copy link
Contributor

risicle commented Oct 10, 2019

I don't know a lot about the nixos tests - @jonringer ?

@jonringer
Copy link
Contributor

I honestly don't know either, but nixos tests allow for you to have many vm's on a virtual network, so they should in theory be able to do cross computer network communication. However, this is probably overkill for just the package, as it would also require nixos modules to configure the VMs initially.

For python packages, you will want to just run the unit tests, this should give a good indication that the package is valid with it's dependencies, without requiring too much setup or compute.

};

propagatedBuildInputs = [ aiohttp pytz ciso8601 ]
++ lib.optional pandasSupport pandas;
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 highly recommended to use pandas, then I would just add this, otherwise leave out the logic to conditionally add. Python packages are weird and it's awkward if two packages have aioinflux as a dependency, then it get's added twice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pandas is one of the killer features of the package, so I'll remove the condition.

Does the double-add happen when one package has pandasSupport, while the other overrides/disables it? Differing buildInputs would mean differing hashes, so different install paths. I can see how that would be non-trivial to solve.

@liamdiprose liamdiprose force-pushed the aioinflux-0.9.0 branch 2 times, most recently from e31c1bf to 0e48fdb Compare November 12, 2019 02:47
@liamdiprose
Copy link
Contributor Author

liamdiprose commented Nov 12, 2019

This rebase is blowing up in my face. Not sure how other peoples commits are seeping in...

Solved, just needed to drop the commit. I'm guessing it'll get picked up in the merge later on.

propagatedBuildInputs = [ aiohttp pytz ciso8601 pandas ];

# Tests require local networking so won't run in the sandbox
doCheck = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

please try to do some tests, you can always scope tests using pytest -k 'not some_test_string'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I agree. I've looked upstream but haven't found any suitable test filters yet. I might contribute a few tests so we can verify the package requirements are right.

I understand @FRidh isn't in a hurry to merge any python packages until we solve the scalability problem. So this isn't a super high priority for me, but I am still interested in getting it merged at some point.

Just letting you know I haven't jumped ship!

Copy link
Contributor

Choose a reason for hiding this comment

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

it's certainly a salability issue for one person, but not as much of an issue for many

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's more solved than I thought then.
I wonder if we could use mypy for a minimal check for all python packages. It can find if there are missing dependencies.

Copy link
Contributor

Choose a reason for hiding this comment

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

looks like mypy package logic differs from standard python mechanisms and makes assumptions as to where the packages should be located. Probably not compatible with nix

https://mypy.readthedocs.io/en/latest/running_mypy.html#how-imports-are-found

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I've had trouble with the logic, and spent alot of time in that section of the mypy docs.

I guess nothing beats actually running the code in the interpreter, and any python package worth packaging will have good test coverage anyway.

Copy link
Contributor

@jonringer jonringer Dec 5, 2019

Choose a reason for hiding this comment

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

you can also do pythonImportsCheck = [ "foo.bar" ]; to essentially run python -c 'import foo.bar'

I use it extensively

pythonImportsCheck = [
to ensure that the azure namespace isn't broken by a rogue __init__.py file

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.

sorry for the very long delay

Comment on lines +3781 to +3786
liamdiprose = {
email = "liam@liamdiprose.com";
github = "liamdiprose";
githubId = 1769386;
name = "Liam Diprose";
};
Copy link
Contributor

Choose a reason for hiding this comment

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

this should be a separate commit

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Roger roger. To be honest I thought I was the slacker 😛

Now that my country is locked down, I see no better option than to get back into open source. I'll be pushing soon.

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 resolve conflicts as well

@stale
Copy link

stale bot commented Sep 23, 2020

Hello, I'm a bot and I thank you in the name of the community for your contributions.

Nixpkgs is a busy repository, and unfortunately sometimes PRs get left behind for too long. Nevertheless, we'd like to help committers reach the PRs that are still important. This PR has had no activity for 180 days, and so I marked it as stale, but you can rest assured it will never be closed by a non-human.

If this is still important to you and you'd like to remove the stale label, we ask that you leave a comment. Your comment can be as simple as "still important to me". But there's a bit more you can do:

If you received an approval by an unprivileged maintainer and you are just waiting for a merge, you can @ mention someone with merge permissions and ask them to help. You might be able to find someone relevant by using Git blame on the relevant files, or via GitHub's web interface. You can see if someone's a member of the nixpkgs-committers team, by hovering with the mouse over their username on the web interface, or by searching them directly on the list.

If your PR wasn't reviewed at all, it might help to find someone who's perhaps a user of the package or module you are changing, or alternatively, ask once more for a review by the maintainer of the package/module this is about. If you don't know any, you can use Git blame on the relevant files, or GitHub's web interface to find someone who touched the relevant files in the past.

If your PR has had reviews and nevertheless got stale, make sure you've responded to all of the reviewer's requests / questions. Usually when PR authors show responsibility and dedication, reviewers (privileged or not) show dedication as well. If you've pushed a change, it's possible the reviewer wasn't notified about your push via email, so you can always officially request them for a review, or just @ mention them and say you've addressed their comments.

Lastly, you can always ask for help at our Discourse Forum, or more specifically, at this thread or at #nixos' IRC channel.

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Sep 23, 2020
@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Oct 3, 2020
@SuperSandro2000 SuperSandro2000 marked this pull request as draft November 28, 2020 00:15
@prusnak prusnak added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Dec 3, 2020
@prusnak
Copy link
Member

prusnak commented Dec 3, 2020

@liamdiprose do you still want to get this in?

@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Dec 3, 2020
@lopsided98
Copy link
Contributor

I submitted an updated version of this PR: #111849

@prusnak
Copy link
Member

prusnak commented Feb 4, 2021

Superseded by #111849

@prusnak prusnak closed this Feb 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants