gapic-generator-python icon indicating copy to clipboard operation
gapic-generator-python copied to clipboard

fix: Break up big files in `types` directory

Open m-strzelczyk opened this issue 3 years ago • 3 comments

Fixes #1252

Dividing the classes for each proto into 3 separate files:

  • %proto/requests.py.j2
  • %proto/responses.py.j2
  • %proto/init.py.j2

The division is pretty simple. All "...Request" classes go to requests.py file, all "...Response" classes go to responses.py and everything else goes to other.py. This way, the content should be spread across multiple files and will move us away from the 2.5Mb PyCharm limit.

For comparison, in current version of python-compute, which I suspect might be the biggest GAPIC API library, the types/compute.py file is 2.8Mb big. After the change, we have:

  • %proto/requests.py - 1.5Mb
  • %proto/responses.py - 15Kb
  • %proto/init.py - 1.4Mb

This change gives us 1Mb distance from the 2.5Mb limit and should last relatively long.

Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

  • [x] Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
  • [x] Ensure the tests and linter pass
  • [x] Code coverage does not decrease (if any source code was changed)
  • [ ] Appropriate docs were updated (if necessary)

Fixes #1252 🦕

m-strzelczyk avatar May 30 '22 14:05 m-strzelczyk

I tested this by replacing google/cloud/compute/types directory in my local python-compute installation by the types directory generated with this change. It worked fine.

m-strzelczyk avatar Jun 09 '22 15:06 m-strzelczyk

Thanks for doing this, @m-strzelczyk ! I have one overall question: what was your rationale for choosing to do a separation based on requests/responses/other as opposed to a separation based on proto service name? Naively, it seems to me the latter keeps related things closer together and is (arguably) more robust if "other" becomes really big itself, or if there is a change to naming conventions.

vchudnov-g avatar Jul 06 '22 22:07 vchudnov-g

@vchudnov-g I agree that it would be nice to sort the types the same way we sort services, however I found it too complicated for me. The types all come from a single file from the googleapis/googleapis repository (in case of Compute that would be file google/cloud/compute/v1/compute.proto). In this file, there's no division, it's just one big bag of types that are used by GCE API. That's why I decided on this arbitrary division into Requests, Responses and others. This division logic is simple and can be easily changed if we decide that it's not working.

m-strzelczyk avatar Jul 13 '22 11:07 m-strzelczyk