Move requests import to runtime #388
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
* Importing requests takes well over 100ms and is used only for resolving refs from remote URIs - move the import to runtime
|
Thanks! I'm on a flight today so might not get a chance to read this carefully until the weekend. I also think we'd want to have a benchmark (in the Really appreciated! |
|
hi @hmehta I'm quite interested in this PR. If I understand, moving the I'm asking because we are using this library pretty heavily in request/response cycles; any small improvement would dramatically impact our general backend performances. Is some perf unit testing is needed, can this PR move forward? It would great to have it merged 🙏 |
|
It's not per validation, this PR is for the one time cost that happens
during import (but still do want to move it along)
…On Mon, Mar 19, 2018, 17:40 apiraino ***@***.***> wrote:
hi @hmehta <https://github.com/hmehta> I'm quite interested in this PR.
If I understand, moving the requests import inside resolve_remote() would
shave ~100ms off each time a schema validation is run, correct?
I'm asking because we are using this library pretty heavily in
request/response cycles; any small improvement would dramatically impact
our general backend performances.
Is some perf unit testing is needed, can this PR move forward? It would
great to have it merged 🙏
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#388 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAUIXtN_SOqiO3rrhBn2k-yXK14ogMnHks5tgCXJgaJpZM4RscDZ>
.
|
|
Merged finally, apologies for the delay... really appreciated! |
tgamblin
added a commit
to spack/spack
that referenced
this pull request
Sep 21, 2019
Spack doesn't need `requests`, and neither does `jsonschema`, but `jsonschema` tries to import it, and it'll succeed if `requests` is on your machine (which is likely, given how popular it is). This commit removes the import to improve Spack's startup time a bit. On a mac with SSD, the import of requests is ~28% of Spack's startup time when run as `spack --print-shell-vars sh,modules` (.069 / .25 seconds), which is what `setup-env.sh` runs. On a Linux cluster where Python is mounted from NFS, this reduces `setup-env.sh` source time from ~1s to .75s. Note: This issue will be eliminated if we upgrade to a newer `jsonschema` (we'd need to drop Python 2.6 for that). See python-jsonschema/jsonschema#388.
tgamblin
added a commit
to spack/spack
that referenced
this pull request
Sep 22, 2019
Spack doesn't need `requests`, and neither does `jsonschema`, but `jsonschema` tries to import it, and it'll succeed if `requests` is on your machine (which is likely, given how popular it is). This commit removes the import to improve Spack's startup time a bit. On a mac with SSD, the import of requests is ~28% of Spack's startup time when run as `spack --print-shell-vars sh,modules` (.069 / .25 seconds), which is what `setup-env.sh` runs. On a Linux cluster where Python is mounted from NFS, this reduces `setup-env.sh` source time from ~1s to .75s. Note: This issue will be eliminated if we upgrade to a newer `jsonschema` (we'd need to drop Python 2.6 for that). See python-jsonschema/jsonschema#388.
tldahlgren
pushed a commit
to tldahlgren/spack
that referenced
this pull request
Oct 11, 2019
Spack doesn't need `requests`, and neither does `jsonschema`, but `jsonschema` tries to import it, and it'll succeed if `requests` is on your machine (which is likely, given how popular it is). This commit removes the import to improve Spack's startup time a bit. On a mac with SSD, the import of requests is ~28% of Spack's startup time when run as `spack --print-shell-vars sh,modules` (.069 / .25 seconds), which is what `setup-env.sh` runs. On a Linux cluster where Python is mounted from NFS, this reduces `setup-env.sh` source time from ~1s to .75s. Note: This issue will be eliminated if we upgrade to a newer `jsonschema` (we'd need to drop Python 2.6 for that). See python-jsonschema/jsonschema#388.
jrmadsen
pushed a commit
to jrmadsen/spack
that referenced
this pull request
Oct 30, 2019
Spack doesn't need `requests`, and neither does `jsonschema`, but `jsonschema` tries to import it, and it'll succeed if `requests` is on your machine (which is likely, given how popular it is). This commit removes the import to improve Spack's startup time a bit. On a mac with SSD, the import of requests is ~28% of Spack's startup time when run as `spack --print-shell-vars sh,modules` (.069 / .25 seconds), which is what `setup-env.sh` runs. On a Linux cluster where Python is mounted from NFS, this reduces `setup-env.sh` source time from ~1s to .75s. Note: This issue will be eliminated if we upgrade to a newer `jsonschema` (we'd need to drop Python 2.6 for that). See python-jsonschema/jsonschema#388.
Julian
added a commit
that referenced
this pull request
Jun 9, 2020
817b724 add perl implementation and test suite to the user list ca14e01 Merge branch 'pull/382' 3dabf55 move non-format tests out of the format directory, while keeping all ECMA regex tests together 4121aa5 move format tests to their own directory 4bae8aa Add more idn-hostname tests to draft-2019-09 and draft-07 6d91158 [325] Add some more hostname tests e593057 Merge pull request #389 from ssilverman/readme-drafts fb3766d README: Improve issue/PR links 79bef22 README: Update language around drafts ade47e4 README: Add Snow to the list of supporting Java validators fc0c14e README: Update simple JSON example 1167669 README: Update structure, consistency, spacing, and wrapping 9514122 Merge pull request #388 from json-schema-org/ether/maxProperties=0 7646490 test that maxProperties = 0 means the object is empty c3f4319 Merge pull request #384 from ChALkeR/chalker/unique 7766f48 Improve uniqueItems validation 7555d41 Add unnormalized $id tests 11f70eb [300] Add tests for valid use of empty fragments in "$id" b106ff0 [299] Add tests for invalid use of fragments in "$id" 4a74d45 Fix "tilde" spelling 3eca41b Merge pull request #379 from json-schema-org/ether/remove-wrapped-refs d61bae8 remove wrapped refs 536ec07 [359] Add unevaluatedProperties/unevaluatedItems cousin tests ac63eb7 Small README update that introduces the concept of directories 697944e Merge pull request #374 from karenetheridge/ether/allOf-anyOf-oneOf 33f8549 test all the *Of keywords together 44b99ed Merge pull request #373 from karenetheridge/ether/items-and-contains 4a2b52f some tests of items + contains 7f00cc8 add test that was present for other drafts but not 2019-09 a3f9e2e Merge pull request #371 from karenetheridge/ether/if-then-else-boolean aeeaecc some tests with if/then/else and boolean schemas b8a083c Merge pull request #372 from nomnoms12/unique-false-and-zero 85728f1 Add tests for uniqueness [1] and [true] fd01a60 Add tests for uniqueness [1] and [true] 0a8823c Merge pull request #370 from karenetheridge/ether/nul-char fa6f4dd add tests for the NUL character in strings 8bf2f7f Merge pull request #369 from ssilverman/data-desc 2ba7a76 Add data description 4f66078 Merge pull request #367 from karenetheridge/ether/additionalItems 283da7c some more tests for additionalItems 7ba95f3 add tests for keywords maxContains, minContains 2f2e7cf Merge pull request #365 from karenetheridge/ether/move-ecma-regex 8388f27 move ECMA regex tests under format/ git-subtree-dir: json git-subtree-split: 817b724b7a64d7c18a8232aa32b5f1cc1d6dd153
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Importing requests takes well over 100ms and is used only for resolving refs from remote URIs - move the import to runtime.
--
Hopefully the test case modification is ok too - mocking a runtime import was not something I've done before :) If there is more elegant or "industry standard" solution, please let me know, and I'll fix it.