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

API overhaul in moving to adc-streaming 1.x #82

Merged
merged 31 commits into from
Jul 3, 2020
Merged

Conversation

myNameIsPatrick
Copy link
Collaborator

@myNameIsPatrick myNameIsPatrick commented Jun 29, 2020

Description

This PR implements a number of changes to adapt to adc-streaming 1.x, as well as additional backwards-incompatible API changes. Since this needed to be done anyways for the upstream package, I took the liberty of making simplifications throughout as needed. This also implements #74 as it was easier wrapping this all up as part of these changes.

Overview:

  • Add new hop auth command with various utilities to create auth configuration and locate the path where this configuration is stored, used in hop publish and hop subscribe to authenticate.
  • Simplify the hop publish and hop subscribe API substantially, removing the -F and -X commands to pass in arbitrary configuration variables. Instead. this is all handled by the auth configuration above and loaded in by default, unless --no-auth is specified. There is also no --timeout arg for hop subscribe, instead it reads messages until EOS is received. In order to open a long-lived connection, you can pass in --persist instead.
  • Move all the functionality from subscribe.classify_message to a Deserializer class in hop.io. This is so the wrapping + serialization (or unwrapping+deserialization) of messages happens within the client itself.
  • Standardize all of the message dataclass methods so that there's consistency between them.
  • Removed this business with if not args: in the entry points since it's basically unused and adds needless complexity.
  • Rewrite many of the docs in general to expose the various functionality. Some of the docs were also stale/incorrect and so I fixed those up as needed as well.
  • Add some unit tests throughout to bring the test coverage above 90% again.

Selected files and motivation for changes:

Makefile:

  • Bumped up the complexity of flake8 to 12 for raising warnings. This is because io.Stream.open() was raising warnings, and it was a bit obnoxious.

README.md:

  • Put all badges on one line
  • Put a link to stable version of RTD rocs

hop/init.py:

  • remove the silent ImportError behavior if _version.py is not found, which was probably more trouble than its worth and not easily testable.

hop/setup.cfg:

  • Ignore unused local variables in the unit tests. I'm inclined to leave these in for clarity even if sometimes they aren't used.

hop/auth.py:

  • Expose various authentication classes from adc-streaming
  • Add CLI utilities. hop auth locate shows where the default auth config location lives, and can be controlled by $XDG_CONFIG_HOME. It uses get_auth_path() to find this location. hop auth setup creates an auth config file in that location if it doesn't already exist, and warns the user and does nothing if it does exist. You can overwrite the existing file with hop auth setup --force.
  • load_auth() loads auth config from that default location and creates an Auth instead, to be passed into a stream instance.

hop/cli.py:

  • Rename add_url_opts to add_client_opts since this also now includes the --no-auth option to be passed in by both publish/subscribe commands.

hop/io.py:

  • Overhaul the streaming functionality for changes to adc-streaming. The easiest way I found to expose most of the same functionality as before was to subclass the Producer and Consumer classes and redefine how messages are serialized and deserialized to do message packing and to return the right message classes seamlessly. A lot of the functionality of subscribe.classify_message has been folded into the Deserializer class, which is used in the consumer. Most of these extra functions/classes, including exposing metadata is private since I don't want to expose any of this to the user.
  • Expose the ConsumerStartPosition from adc-streaming to control the message offset while not having to call adc directly.

hop/models.py:

  • Add feature parity to all the message dataclasses, renaming all the loading functionality to load() and load_file(), which should hopefully be more straightforward to handle message loading in the same way across all of these.
  • Rename wrap_message() to serialize() since this may actually serialize in the future and provide some future-proofing.

hop/publish.py:

  • Control the --format argument options with the members of io.Deserializer. This is so users are required to choose between these options.

hop/subscribe.py:

  • Swap --earliest with a --start-at, with the same enum control as with --format above. To specify earliest, one can do --start-at EARLIEST now.
  • Remove --timeout. By default, this will listen until EOS. If a long-lived connection is desired, the --persist command is exposed.

Extra notes

  • While looking at the checklist below, I may have some indentation stuff to fix relating to the docstrings which I'll be fixing later today. (now fixed)

Checklist

  • All new functions and classes are documented and adhere to Google doc style (3.8.3-3.8.6 of this document)
  • Add/update sphinx documentation with any relevant changes.
  • Add/update pytest-style tests in /tests, ensuring sufficient code coverage.
  • make test runs without errors.
  • make lint doesn't give any warnings.
  • make format doesn't give any code formatting suggestions.
  • make doc runs without errors and generated docs render correctly.
  • Check that CI pipeline run on this PR passes all stages.
  • Review signoff by at least one developer.

NOTE: If this PR relates to a release, open and reference an issue with the Release checklist template.

Copy link
Contributor

@spenczar spenczar left a comment

Choose a reason for hiding this comment

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

First round of review is done. I haven't reviewed tests and haven't reviewed all the doc changes yet.

This looks really good - a bunch of major improvements here to the clarity and consistency of the code. Nice work!

README.md Outdated Show resolved Hide resolved
doc/user/quickstart.rst Show resolved Hide resolved
hop/io.py Outdated Show resolved Hide resolved
hop/io.py Outdated Show resolved Hide resolved
hop/io.py Show resolved Hide resolved
hop/io.py Outdated Show resolved Hide resolved
hop/auth.py Outdated Show resolved Hide resolved
hop/auth.py Outdated Show resolved Hide resolved
hop/auth.py Show resolved Hide resolved
hop/auth.py Outdated Show resolved Hide resolved
Copy link
Contributor

@spenczar spenczar left a comment

Choose a reason for hiding this comment

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

Few notes on testing added. I think that we should get integration tests in here eventually, although now is probably not the time.

tests/test_io.py Show resolved Hide resolved
tests/test_io.py Show resolved Hide resolved
@myNameIsPatrick
Copy link
Collaborator Author

Thanks @spenczar for the thorough review! I'll start incorporating some of those changes you requested.

Copy link
Contributor

@bfc5288 bfc5288 left a comment

Choose a reason for hiding this comment

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

I left a few suggestions in the code so far -- I'll add more as I dig deeper and do some testing.

doc/user/commands.rst Outdated Show resolved Hide resolved
hop/models.py Show resolved Hide resolved
tests/test_io.py Outdated Show resolved Hide resolved
setup.py Show resolved Hide resolved
hop/io.py Outdated
if format in cls.__members__:
return cls[format.upper()].value(**content)
else:
raise ValueError(f"Message format {format} not recognized")
Copy link
Contributor

Choose a reason for hiding this comment

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

@spenczar A fallback to BLOB happens when publishing an unspecified message -- it may be beneficial to have another fallback here, though I'm not sure if we need that redundancy in the internal Deserializer.

hop/io.py Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Jun 30, 2020

Codecov Report

Merging #82 into master will increase coverage by 4.69%.
The diff coverage is 89.69%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #82      +/-   ##
==========================================
+ Coverage   86.99%   91.69%   +4.69%     
==========================================
  Files           8        9       +1     
  Lines         223      325     +102     
==========================================
+ Hits          194      298     +104     
+ Misses         29       27       -2     
Flag Coverage Δ
#pytest 91.69% <89.69%> (+4.69%) ⬆️
Impacted Files Coverage Δ
hop/auth.py 78.43% <78.43%> (ø)
hop/models.py 90.00% <85.71%> (+0.29%) ⬆️
hop/subscribe.py 90.90% <88.88%> (+4.24%) ⬆️
hop/io.py 94.39% <94.23%> (-5.61%) ⬇️
hop/__init__.py 100.00% <100.00%> (+28.57%) ⬆️
hop/__main__.py 97.14% <100.00%> (+0.26%) ⬆️
hop/cli.py 100.00% <100.00%> (+14.28%) ⬆️
hop/publish.py 100.00% <100.00%> (+17.85%) ⬆️
hop/version.py 100.00% <100.00%> (+42.85%) ⬆️
... and 1 more

Copy link
Contributor

@spenczar spenczar left a comment

Choose a reason for hiding this comment

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

I'm pretty sure all my changes have been addressed, looks great.

Copy link
Contributor

@bfc5288 bfc5288 left a comment

Choose a reason for hiding this comment

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

This is looking good, I resolved the previous comments. I did some CLI testing and found a few points, added in-line.

I definitely agree that integration testing should be one of the next steps as it'll be easier to find bugs like this.

hop/publish.py Outdated Show resolved Hide resolved
hop/models.py Show resolved Hide resolved
hop/publish.py Show resolved Hide resolved
hop/models.py Show resolved Hide resolved
@bfc5288
Copy link
Contributor

bfc5288 commented Jul 3, 2020

This looks ready to me. Excellent work putting everything together! Looking forward to doing a release of all these changes soon.

@myNameIsPatrick
Copy link
Collaborator Author

Thanks @bfc5288 and @spenczar for the very thorough review and good discussions that came out of this, including new user stories (#84, #85 and #86)!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants