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

pythonPackages.google-cloud-*: init all 37 modules #49657

Merged
merged 37 commits into from Nov 21, 2018

Conversation

costrouc
Copy link
Member

@costrouc costrouc commented Nov 2, 2018

Motivation for this change

I will be doing a lot of development with google-cloud in the coming year. Wanted to make sure that all the google python modules are available.

Things done

Tests are enabled for every package. All pass entirely. Compared to azure and aws python tools this is the best set of modules I have seen. No circular dependencies, pytest and mock are the only testing dependencies.

Many modules have been added. Without this python module initialization tools I made this would have taken days.

  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • 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 nox --run "nox-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)
  • Fits CONTRIBUTING.md.

@costrouc costrouc requested a review from FRidh as a code owner November 2, 2018 20:47
@costrouc
Copy link
Member Author

costrouc commented Nov 2, 2018

Google removed a meta package that installed all of the packages (meta package google-cloud) without having to specify them individually. Would this be worth adding?

@costrouc costrouc changed the title Add all python google-cloud-* modules pythonPackages.google-cloud-*: init all 37 modules Nov 2, 2018
@FRidh
Copy link
Member

FRidh commented Nov 3, 2018

How do you envision maintaining, that is, updating these packages? @r-ryantm only updates the source, whereas we might want to update more things. Maybe it's an idea to generate with your tool generated.nix, and then use .overridePythonAttrs in default.nix? There is a memory consideration here but it would allow for more automated maintenance.

Meta packages are not worth adding until another package requires them.

@costrouc
Copy link
Member Author

costrouc commented Nov 3, 2018

Definitely something that I would be interested looking into. You are talking about doing something similar to how haskell is handled in nixpkgs? So as of right now I can think of two issues.

  1. Currently there is no pattern followed for naming python packages e.g. is it (google-cloud-datastore vs google_cloud_datastore)? This may break some peoples setups but it would allow automation of package names and searching for packages easier if we had a rule for naming package attributes and folder names. I could see name.lowercase().replace('_', '-').replace('.', '-') . My tool had a hard time getting the package names correct.

  2. Running tests. I had to manually add all the tests and the dependencies. There are just too many ways that testing is done in python. I can't think of any solution to this problem. But if we are handling updates this might not be a problem.

I think that 1 is the main blocker. We would just need to establish a convention. With that advice I will not add the meta package google-cloud.

@FRidh
Copy link
Member

FRidh commented Nov 3, 2018

  1. the naming guideline is to use normalized names, but many don't follow it.
  2. That we indeed cannot automate yet.

For Haskell we generate a whole package set in one file, and then have a file with overrides per Haskell/GHC version. This is something that was discussed in the past as well for Python. In the end, I gave up on that direction. For Haskell we base our package sets on the curated Stackage sets. For Python, such sets don't exist so we need to resolve a stable set first. When resolving, we need to consider all Python versions and platforms. Unfortunately, it is not possible to retrieve PyPI at a certain state, making it even harder to resolve. Now, these problems can be resolved, one by one.

What I was thinking of now was maybe a generated.nix and default.nix file per package, but I am not sure whether it is worth making this change.

@costrouc
Copy link
Member Author

costrouc commented Nov 3, 2018

I know this would require a lot of rebuilds and would possibly break people's configuration but I think that getting all the python package names in nixpkgs normalized is a first step before update automation could be considered.

I think that a generated.nix is very doable and a great idea! With correct normalized package naming the only thing this tool this tool misses is extra packages and checkInputs. default.nix would get a lot shorter and simpler to write.

I would be willing to work on normalizing python package attributes and directories. And later possibly a generated.nix for packages.

@costrouc
Copy link
Member Author

Bump. Ready for merge. Only added new packages and will allow access to the newer google-cloud python apis.

@costrouc costrouc force-pushed the costrouc/python-google-cloud-modules branch from 72ec1e6 to 3fe92cc Compare November 16, 2018 01:03
@costrouc
Copy link
Member Author

@Mic92 showed me nix-review was now able to confirm that all packages work without nix-build -A <attribute> over and over. Ready for merge.

@costrouc costrouc force-pushed the costrouc/python-google-cloud-modules branch from 3fe92cc to 24d96d9 Compare November 20, 2018 19:12
@costrouc
Copy link
Member Author

Rebased to fix merge conflict. @FRidh is there anything that I can do to help speed up the python modules merge process? Should I ask others for reviews on python contributions? How can I help? I understand you are quite busy and I don't want to make work harder for you. I have tried to combine commits so that the review process is quicker but it doesn't always seem that works. I am committed to nixpkgs and I want to see it succeed.

nix-review has given me much more confidence in my PRs as before I was nix-build -A ... for each attribute manually. So I can say from this point on I will have a much greater assurance that my PRs are good.

@Mic92 Mic92 merged commit 97c4229 into NixOS:master Nov 21, 2018
@costrouc
Copy link
Member Author

@Mic92 do you mind if I ping you on python merge requests that take awhile to merge? Usually @FRidh is the only one that gets pinged.

@Mic92
Copy link
Member

Mic92 commented Nov 21, 2018

@costrouc Since I have installed github-refined, I will see pull requests order by last update. This means I will also see older pull request that got updated. I take this list as a priority. I also look at pings but not as regular as this one. Apart from that you can also ask around in IRC. Usually it is also a good idea to keep your pull requests smaller then this one so one can review it faster.

@costrouc
Copy link
Member Author

Will do in the future!

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

4 participants