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

Use asyncio to speed up cloud identification #12

Merged
merged 6 commits into from
Apr 25, 2022

Conversation

kshivakumar
Copy link
Collaborator

@kshivakumar kshivakumar commented Jan 7, 2022

Implements #11

Uses asyncio to check all the cloud providers concurrently and returns back a response as soon as the cloud provider is detected.

Important changes:

  • Replaced requests with aiohttp in the dependencies
  • Removed Python 3.4 and 3.5 support.
  • Added identifier class variable to each *Provider class
  • Added SUPPORTED_PROVIDERS api - it's a tuple of identifier values of each Provider class
  • Added support for Python 3.9 - the code and test cases are working

I manually tested the changes in Azure and GCP, it's working fine and the response comes back within a second.

@kshivakumar
Copy link
Collaborator Author

kshivakumar commented Jan 7, 2022

For some unknown reason, I get the below error on my machine(MacOS x86) during pre-commit/make test:

Reorder python imports...................................................Failed
- hook id: reorder-python-imports
- exit code: 1

Traceback (most recent call last):
  File "/root/.cache/pre-commit/repol5aizj_t/py_env-python3/bin/reorder-python-imports", line 8, in <module>
    sys.exit(main())
  File "/root/.cache/pre-commit/repol5aizj_t/py_env-python3/lib/python3.7/site-packages/reorder_python_imports.py", line 685, in main
    application_directories=args.application_directories.split(':'),
  File "/root/.cache/pre-commit/repol5aizj_t/py_env-python3/lib/python3.7/site-packages/reorder_python_imports.py", line 420, in fix_file_contents
    partitioned = step(partitioned)
  File "/root/.cache/pre-commit/repol5aizj_t/py_env-python3/lib/python3.7/site-packages/reorder_python_imports.py", line 334, in apply_import_sorting
    sorted_blocks = sort(import_obj_to_partition.keys(), **sort_kwargs)
  File "/root/.cache/pre-commit/repol5aizj_t/py_env-python3/lib/python3.7/site-packages/aspy/refactor_imports/sort.py", line 96, in sort
    imports_partitioned[classify_func(import_obj)].append(import_obj)
  File "/root/.cache/pre-commit/repol5aizj_t/py_env-python3/lib/python3.7/site-packages/aspy/refactor_imports/sort.py", line 73, in classify_func
    return classify_import(obj.import_statement.module, **kwargs)
  File "/root/.cache/pre-commit/repol5aizj_t/py_env-python3/lib/python3.7/site-packages/aspy/refactor_imports/classify.py", line 139, in classify_import
    base, application_directories,
  File "/root/.cache/pre-commit/repol5aizj_t/py_env-python3/lib/python3.7/site-packages/aspy/refactor_imports/classify.py", line 103, in _get_module_info
    assert spec.submodule_search_locations is not None
AssertionError

I tried different Python versions (3.7, 3.8, 3.9), but the error doesn't go away.
Other hooks and test cases are all passing.

Update: The error occurs in setup.py file


if tasks:
await cancel_unfinished_tasks()
return 'timeout'
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If the code runs in a non-supported cloud environment or in local, the response is usually going to betimeout because almost all the cloud provider urls take more than 5s to respond.
My original motivation for adding timeout in addition to unknown and cloud-name was being explicit, but now I feel it makes the api more complicated for the user. I am mulling over to remove timeout and replace it with unknown. @dgzlopes Let me know your thoughts on this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In my latest commits I have set timeout's default value as None and removed "timeout" as a response.
So, by default, provider will take all its time to determine the cloud provider. In my local, it took around 1 min 15s to return unknown.

return result


async def async_provider(timeout=TIMEOUT):
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If the user code is async, then they should use this api instead of provider. I will mention the same in docstrings.

@dgzlopes dgzlopes self-requested a review January 11, 2022 10:39
@kshivakumar
Copy link
Collaborator Author

kshivakumar commented Jan 11, 2022

@dgzlopes Test cases are failing for Python 3.5 because the library for mocking api responses - aresponses doesn't support Python 3.5. The library uses format strings which was added in 3.6.
I have to check if there's an alternative library that supports Python 3.5.

@kshivakumar
Copy link
Collaborator Author

kshivakumar commented Jan 11, 2022

@dgzlopes How important is Python 3.5 support? It's officially retired - https://endoflife.date/python.
I initially thought of suggesting to remove support for Python 3.6 and below. The asynchronous api in Python kind of stabilised in Python 3.7 and has remained consistent till 3.9.

@dgzlopes
Copy link
Owner

Not important @kshivakumar :D

Feel free to drop it!

@kshivakumar kshivakumar force-pushed the use_async branch 2 times, most recently from 55ffdb9 to 0e202dd Compare January 19, 2022 08:03
@dgzlopes
Copy link
Owner

I'll review the PR at the end of the week (I've scheduled it on my personal calendar).

Sorry for my slow pace 😄

Copy link
Owner

@dgzlopes dgzlopes left a comment

Choose a reason for hiding this comment

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

Thanks again for the PR! LGTM :)

The only thing, while running tox locally, the pre-commit hook step did some fixes on setup.py (re-ordering Python import). Do you mind committing those?

Thanks again!

Signed-off-by: kshivakumar <kshivakumar@outlook.com>
Signed-off-by: kshivakumar <kshivakumar@outlook.com>
Signed-off-by: kshivakumar <kshivakumar@outlook.com>
Signed-off-by: kshivakumar <kshivakumar@outlook.com>
Signed-off-by: kshivakumar <kshivakumar@outlook.com>
Signed-off-by: kshivakumar <kshivakumar@outlook.com>
@kshivakumar
Copy link
Collaborator Author

kshivakumar commented Feb 7, 2022

The only thing, while running tox locally, the pre-commit hook step did some fixes on setup.py (re-ordering Python import). Do you mind committing those?

@dgzlopes Fixed it and also updated README.

@kshivakumar
Copy link
Collaborator Author

@dgzlopes Can we merge this if there are no further issues?

@dgzlopes dgzlopes merged commit 5322a58 into dgzlopes:master Apr 25, 2022
@dgzlopes
Copy link
Owner

Sorry for the delay! I had a mess on my GH notifications :(

Merged. I'll cut a new release.

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.

None yet

2 participants