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

Sparsezoo stdout fix #322

Merged
merged 3 commits into from
May 31, 2023
Merged

Sparsezoo stdout fix #322

merged 3 commits into from
May 31, 2023

Conversation

rahul-tuli
Copy link
Member

@rahul-tuli rahul-tuli commented May 31, 2023

This PR reverts commit https://github.com/neuralmagic/sparsezoo/commit/960cae23e8273cc7e3cfff9ceb65a8f3ecb8628f.

The newly added GoogleAnalytics class was throwing 429 Client Errors: a proposed fix for that was to suppress sys.stdout and sys.stderr streams in a multithreaded function, The problem with this fix was that the streams were not being restored properly and were suppressing output from all commands, and raised a 120 error code a potential fix could be to

  • Execute this function in a single thread of execution, in which case the streams would be restored properly but we would lose the benefits of multithreading ❌
  • Changing code to include a flush method in the NullDevice class, this would return an error code of zero but would still suppress all output ❌
  • The proposed fix should have never made it in as the code was already handling all exceptions raised by the request ✅ + the fix was solving a plumbing problem rather than addressing the original issue.

I wasn't able to reproduce the exact error 429 Client Error from GoogleAnalytics request but raised a dummy HTTPError myself and verfied the the code actually catches all HTTPErrors, on another note this PR also makes the exception catching more specific over catching all Exceptions

If the 429 Client Errors re-appear they should be handled by the current code; otherwise should be addressed differently preferrably in the _send_request function without redirection of stdout/stderr streams.

$ deepsparse.check_hardware              
GenuineIntel CPU detected with 10 cores. (1 sockets with 10 cores each)
DeepSparse FP32 model performance supported: True.
DeepSparse INT8 (quantized) model performance supported: TRUE (emulated).

Non VNNI system detected. Performance speedups for INT8 (quantized) models is available, but will be slower compared with a VNNI system. Set NM_FAST_VNNI_EMULATION=True in the environment to enable faster emulated inference which may have a minor effect on accuracy.

Additional CPU info: {'L1_data_cache_size': 32768, 'L1_instruction_cache_size': 32768, 'L2_cache_size': 1048576, 'L3_cache_size': 14417920, 'architecture': 'x86_64', 'available_cores_per_socket': 10, 'available_num_cores': 10, 'available_num_hw_threads': 20, 'available_num_numa': 1, 'available_num_sockets': 1, 'available_sockets': 1, 'available_threads_per_core': 2, 'bf16': False, 'cores_per_socket': 10, 'dotprod': False, 'i8mm': False, 'isa': 'avx512', 'num_cores': 10, 'num_hw_threads': 20, 'num_numa': 1, 'num_sockets': 1, 'threads_per_core': 2, 'vbmi': False, 'vbmi2': False, 'vendor': 'GenuineIntel', 'vendor_id': 'Intel', 'vendor_model': 'Intel(R) Core(TM) i9-7900X CPU @ 3.30GHz', 'vnni': False, 'zen1': False}

@markurtz markurtz merged commit 9b5a0bd into main May 31, 2023
@markurtz markurtz deleted the sparsezoo-stdout-fix branch May 31, 2023 17:19
rahul-tuli added a commit that referenced this pull request May 31, 2023
* Revert "Suppress analytics errors and messages (#318)"

This reverts commit 960cae2.

* Catch HttpError over exception

* Catch HttpError over exception when checking country

(cherry picked from commit 9b5a0bd)
KSGulin pushed a commit that referenced this pull request Jun 1, 2023
* Revert "Suppress analytics errors and messages (#318)"

This reverts commit 960cae2.

* Catch HttpError over exception

* Catch HttpError over exception when checking country

(cherry picked from commit 9b5a0bd)
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.

2 participants