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.

Sign up for GitHub

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

Jump to bottom

qhub/initialize: lazy load attributes that require remote information #1509

Merged
merged 8 commits into from
Oct 26, 2022
Merged

qhub/initialize: lazy load attributes that require remote information #1509

merged 8 commits into from
Oct 26, 2022

Conversation

FFY00
Copy link
Contributor

@FFY00 FFY00 commented Oct 25, 2022

Fixes #1506

Changes introduced in this PR:

This patch caches set_docker_image_tag and set_qhub_dask_version, since these functions require network access, which is expensive, and makes qhub.initilize lazy load the attributes that depend on them.

Types of changes

What types of changes does your PR introduce?

Put an x in the boxes that apply

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds a feature)
  • Breaking change (fix or feature that would cause existing features to not work as expected)
  • Documentation Update
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no API changes)
  • Build related changes
  • Other (please describe):

Testing

Requires testing

  • Yes
  • No

In case you checked yes, did you write tests?

  • Yes
  • No

Documentation

Does your contribution include breaking changes or deprecations?
If so have you updated the documentation?

  • Yes, docstrings
  • Yes, main documentation
  • Yes, deprecation notices

Further comments (optional)

The way this is implemented isn't very pretty, but I didn't want to change the code too much. As I am not familiar with this code, I don't feel comfortable enough refactoring it to better handle these cases.

@trallard trallard requested a review from viniciusdc October 25, 2022 15:02
@trallard trallard added needs: review 👀 This PR is complete and ready for reviewing area: user experience 👩🏻‍💻 area: dependencies 📦 All things dependencies area: nebari-cli type: bug 🐛 Something isn't working labels Oct 25, 2022
@iameskild iameskild changed the base branch from main to release/2022.10.1 October 25, 2022 17:54
@viniciusdc
Copy link
Contributor

I will have a look, thanks @FFY00, for the contribution !!!

@trallard trallard added this to the 🪴 Nebari rename milestone Oct 26, 2022
nebari-sensei
nebari-sensei reviewed Oct 26, 2022
View reviewed changes
nebari/initialize.py Outdated Show resolved Hide resolved
nebari-sensei
nebari-sensei reviewed Oct 26, 2022
View reviewed changes
nebari/initialize.py Outdated Show resolved Hide resolved
viniciusdc
viniciusdc approved these changes Oct 26, 2022
View reviewed changes
Copy link
Contributor

@viniciusdc viniciusdc left a comment

Choose a reason for hiding this comment

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

LGTM tested with a local init and some commands (--help, --version) all working!!!. Thanks, @FFY00 and @aktech, for this!

@viniciusdc viniciusdc merged commit bc0cbed into nebari-dev:release/2022.10.1 Oct 26, 2022
@aktech aktech deleted the initialize-lazy-load branch October 26, 2022 19:57
iameskild added a commit that referenced this pull request Nov 2, 2022
@iameskild @pavithraes
@viniciusdc @trallard @FFY00 @aktech @nebari-sensei
Release/2022.10.1 (#1527)
788aae8 
* Switch to CalVer (#1501)

* Run CI on branchs release/**

* Update pyproject.toml to use calver

* Update on branch

* Update theme welcome messages to use Nebari (#1503)

* update welcome messge

* remove qhub specifics from theme

* Name change QHub --> Nebari (#1508)

* Name change: .github

* Name change: qhub/

* Name change: tests/

* Name change: tests_deployment/

* Name change: tests_e2e/

* Name change: /

* Rename folder

* Rename releases

* Remove qhub/

* Run pre-commit

* Revert docs links

* Fix infracost artifact names

* Update upgrade command + test

* Updates based on review

* Update step output format

* Fix: Update step output format

* Update nebari/provider/cicd/common.py

Co-authored-by: Tania Allard <taniar.allard@gmail.com>

* Update stage 08 folder names

* Update nebari/schema.py

Co-authored-by: Tania Allard <taniar.allard@gmail.com>

* Update nebari/template/stages/02-infrastructure/azure/variables.tf

Co-authored-by: Tania Allard <taniar.allard@gmail.com>

* Update nebari/template/stages/02-infrastructure/do/variables.tf

Co-authored-by: Tania Allard <taniar.allard@gmail.com>

* Update nebari/template/stages/02-infrastructure/gcp/variables.tf

Co-authored-by: Tania Allard <taniar.allard@gmail.com>

* Update nebari/template/stages/02-infrastructure/gcp/variables.tf

Co-authored-by: Tania Allard <taniar.allard@gmail.com>

* Update nebari/template/stages/03-kubernetes-initialize/variables.tf

Co-authored-by: Tania Allard <taniar.allard@gmail.com>

* Update nebari/template/stages/05-kubernetes-keycloak/variables.tf

Co-authored-by: Tania Allard <taniar.allard@gmail.com>

* Update nebari/template/stages/06-kubernetes-keycloak-configuration/variables.tf

Co-authored-by: Tania Allard <taniar.allard@gmail.com>

* Update nebari/template/stages/06-kubernetes-keycloak-configuration/variables.tf

Co-authored-by: Tania Allard <taniar.allard@gmail.com>

* Update nebari/template/stages/07-kubernetes-services/modules/kubernetes/forwardauth/variables.tf

Co-authored-by: Tania Allard <taniar.allard@gmail.com>

* Update setup.cfg

Co-authored-by: Tania Allard <taniar.allard@gmail.com>

* Update scripts/aws-force-destroy.py

Co-authored-by: Tania Allard <taniar.allard@gmail.com>

* Update setup.cfg

Co-authored-by: Tania Allard <taniar.allard@gmail.com>

* Update setup.cfg

Co-authored-by: Tania Allard <taniar.allard@gmail.com>

* Update setup.cfg

Co-authored-by: Tania Allard <taniar.allard@gmail.com>

* Update setup.cfg

Co-authored-by: Tania Allard <taniar.allard@gmail.com>

* Update nebari/constants.py

Co-authored-by: Vinicius D. Cerutti <51954708+viniciusdc@users.noreply.github.com>

* Update setup.cfg

Co-authored-by: Tania Allard <taniar.allard@gmail.com>

* fix flake8 complaint

* Update ref to qhub-dask to nebari-dask

* Update readme

* Change to PYPI_API_TOKEN

Co-authored-by: viniciusdc <vinivdc2009@hotmail.com>
Co-authored-by: Tania Allard <taniar.allard@gmail.com>
Co-authored-by: Vinicius D. Cerutti <51954708+viniciusdc@users.noreply.github.com>

* qhub/initialize: lazy load attributes that require remote information (#1509)

* qhub/initialize: lazy load attributes that require remote information

* Fix broken configuration

* fix renaming

* fix header text

* Update nebari/initialize.py

* Update nebari/initialize.py

* Blackify

Co-authored-by: Amit Kumar <dtu.amit@gmail.com>
Co-authored-by: Nebari-sensei <116370392+nebari-sensei@users.noreply.github.com>

* Update README logo (#1514)

* Add fix, enhancements and pytests for CLI (#1498)

* Add fix, enhancements and pytests for CLI

* No zero-padded month for CalVer release branches

* Update cli test

* Simplify test.yaml

* Remove old CLI + cleanup (#1519)

* Remove old cli

* Update theme

* Remove shim

* display_version: True

* Update `skip_remote_state_provision` default value (#1521)

* Add release notes for 2022.10.1 (#1523)

* Add release notes for 2022.10.1

* Wording

* Update README.md with new docs links

* Update README.md with new docs links

Co-authored-by: Vinicius D. Cerutti <51954708+viniciusdc@users.noreply.github.com>

* Remove leading `v` for release action (#1526)

* Remove leading v for release tags

* Add note to release checklist

* Merge release branch into main

Co-authored-by: Pavithra Eswaramoorthy <pavithraes@outlook.com>
Co-authored-by: viniciusdc <vinivdc2009@hotmail.com>
Co-authored-by: Tania Allard <taniar.allard@gmail.com>
Co-authored-by: Vinicius D. Cerutti <51954708+viniciusdc@users.noreply.github.com>
Co-authored-by: Filipe Laíns <filipe.lains@gmail.com>
Co-authored-by: Amit Kumar <dtu.amit@gmail.com>
Co-authored-by: Nebari-sensei <116370392+nebari-sensei@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nebari-sensei nebari-sensei nebari-sensei left review comments

@viniciusdc viniciusdc viniciusdc approved these changes

Assignees
No one assigned
Labels
area: dependencies 📦 All things dependencies area: nebari-cli area: user experience 👩🏻‍💻 needs: review 👀 This PR is complete and ready for reviewing type: bug 🐛 Something isn't working
Projects
None yet
Milestone
🪴 Nebari rename
Development

Successfully merging this pull request may close these issues.

[BUG] - CLI takes a long time to load
5 participants
@FFY00 @viniciusdc @nebari-sensei @trallard @aktech