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

_discover_services keyword in FDSN Client #2325

Merged
merged 7 commits into from Mar 15, 2019

Conversation

@jkmacc-LANL
Copy link
Contributor

jkmacc-LANL commented Feb 14, 2019

What does this PR do?

This PR adds a discover_services boolean keyword to obspy.clients.fdsn.Client.__init__.

Client._discover_services() can be an expensive operation for a data
center, but it was run on Client.__init__ even if it wasn't used. This
keyword bypasses the service discover/query, and instead assumes the
default services at the data center. This is helpful for distributed
applications that instantiate many clients in seperate Python processes.

Why was it initiated? Any relevant Issues?

See #2094 for a short discussion. (Note: an additional PR is necessary to end that issue.)

This PR branches from a recent master branch.

PR Checklist

  • Correct base branch selected? master for new features, maintenance_... for bug fixes
  • This PR is not directly related to an existing issue (which has no PR yet).
  • If any network modules should be tested for the PR, add them as a comma separated list
    (e.g. clients.fdsn,clients.arclink) after the colon in the following magic string: "+TESTS:"
    (you can also add "ALL" to just simply run all tests across all modules)
  • All tests still pass.
  • Any new features or fixed regressions are be covered via new tests.
  • Any new or changed features have are fully documented.
  • Significant changes have been added to CHANGELOG.txt .

@jkmacc-LANL jkmacc-LANL self-assigned this Feb 15, 2019

@krischer
Copy link
Member

krischer left a comment

By and large I'm fine with this. Two comments:

  • Please call the argument _discover_services so we don't commit to keeping it around. Also in the documentation please mention that this will likely go away at point. I think we can solve this completely by only requesting the WADLs if non-default parameters are passed but that is more work and won't happen for this release.

  • The tests don't explicitly test that the WADLs are not requested. Should be quite easy to do with some mocks but then I don't care that much as this is hopefully only temporary functionality. So I'd leave this up to you.

@jkmacc-LANL

This comment has been minimized.

Copy link
Contributor Author

jkmacc-LANL commented Feb 15, 2019

Thanks, Lion.

  • Yeah, I agree. I'll do that.
  • I don't know mocks, really. I was working on a solution that stuck a fake .services dictionary onto the client that contained some reasonable default parameters. Not my greatest work 💩

Both suggestions are workarounds for the real solution, however. What do you think it would take to check for non-default parameters from the user? I'm willing to try something out if it can avoid doing something would have to be undone.

@krischer

This comment has been minimized.

Copy link
Member

krischer commented Feb 15, 2019

Yea - I think the simplest way for a proper solution would be to just check if only default parameters have been passed to all the functions and then do what you already did and otherwise call the normal ._discover_services() method. Some more logic is probably required but that should do the trick.

@jkmacc-LANL

This comment has been minimized.

Copy link
Contributor Author

jkmacc-LANL commented Feb 15, 2019

Ok, so:

  1. Skip ._discover_services() at __init__.
  2. In each method, skip ._discover_services if the passed params are the defaults for that method/service, otherwise call ._discover_services and do the normal if "service_name" in self.services check.

I wonder if I could do the lazy services check @nick-iris requested by adding a keyword like ._discover_services(service='dataselect'), thereby splitting up the WADL checks into their respective methods and avoiding unused service queries.

@megies megies added this to the 1.2.0 milestone Feb 15, 2019

@megies megies added this to In Progress in Release 1.2.0 Feb 15, 2019

@jkmacc-LANL

This comment has been minimized.

Copy link
Contributor Author

jkmacc-LANL commented Feb 15, 2019

check if only default parameters have been passed to all the functions and then do what you already did and otherwise call the normal ._discover_services() method

I just realized that ._discover_services would still be called if non-default parameters were specified, which doesn't bypass the potentially expensive service discovery queries if non-default parameters were desired. The PR is attempting to provide a "fast, unsafe, production" mode for the Client.

I think I'm now leaning back towards the original discover_services=True flag that, if False simply replaces self.services with sensible defaults. I don't know if _create_url_from_parameters would break, but maybe worth a shot.

@krischer

This comment has been minimized.

Copy link
Member

krischer commented Feb 15, 2019

I think I'm now leaning back towards the original discover_services=True flag that, if False simply replaces self.services with sensible defaults. I don't know if _create_url_from_parameters would break, but maybe worth a shot.

Fine with me as long we the argument is called _discover_services=True and its documented that we don't commit to maintaining the API.

I unfortunately won't have time to look at this properly during the next couple of days.

@jkmacc-LANL jkmacc-LANL force-pushed the discover_services branch 2 times, most recently from 4f9ac4b to eb68c34 Feb 15, 2019

@jkmacc-LANL jkmacc-LANL changed the title WIP: discover_services keyword in FDSN Client _discover_services keyword in FDSN Client Feb 16, 2019

@jkmacc-LANL

This comment has been minimized.

Copy link
Contributor Author

jkmacc-LANL commented Feb 16, 2019

+TESTS:clients.fdsn

@megies

This comment has been minimized.

Copy link
Member

megies commented Feb 18, 2019

+TESTS:clients.fdsn

need to update the PR to make this have an effect (i usually amend the last last commit and force push)

@jkmacc-LANL jkmacc-LANL force-pushed the discover_services branch from eb68c34 to 0e9e557 Feb 18, 2019

@jkmacc-LANL

This comment has been minimized.

Copy link
Contributor Author

jkmacc-LANL commented Feb 18, 2019

Could I put it in the commit message, too?

@jkmacc-LANL

This comment has been minimized.

Copy link
Contributor Author

jkmacc-LANL commented Feb 22, 2019

Thanks @megies .

PS. I don't think I currently have any tasks on this PR. Please let me know if you intend me to make further changes, otherwise I'm hoping it might be considered for 1.2.0.

jkmacc-LANL added some commits Feb 14, 2019

Add discover_services boolean keyword to Client init
Client._discover_services() can be an expensive operation for a data
center, but it was run on Client.__init__ even if it wasn't used.  This
keyword bypasses the service discover/query, and instead assumes the
default services at the data center.  This is helpful for distributed
applications that instantiate many clients in seperate Python processes.
Implement _discover_services=False
We build a minimal and very permissive DEFAULT_SERVICES dictionary to
be used in place of the actual WADL parsing services query on __init__.
This allows the normal machinery that uses client.services, like URL
building and type checking, but doesn't bother the data center with the
services query.
Add tests for _discover_services=False
Copied the IRIS query tests, but using the new fake services dictionary.
Update changelog
flake8 stuff, too

@megies megies force-pushed the discover_services branch from 0e9e557 to f91224f Mar 15, 2019

@megies

This comment has been minimized.

Copy link
Member

megies commented Mar 15, 2019

Rebased and force-pushed to get fresh CI

@megies megies moved this from In Progress to Waiting on CI in Release 1.2.0 Mar 15, 2019

krischer added some commits Mar 15, 2019

@krischer

This comment has been minimized.

Copy link
Member

krischer commented Mar 15, 2019

Adapted some tests to the latest changes in the master.

@krischer krischer merged commit 4d35268 into master Mar 15, 2019

0 of 3 checks passed

ci/circleci CircleCI is running your tests
Details
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

@krischer krischer deleted the discover_services branch Mar 15, 2019

@krischer krischer moved this from Waiting on CI to Done in Release 1.2.0 Mar 15, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.