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

Retrieve all conda-store environments #2910

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

soapy1
Copy link
Contributor

@soapy1 soapy1 commented Jan 14, 2025

Reference Issues or PRs

Fixes #2599

What does this implement/fix?

Put a 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 not to work as expected)
  • Documentation Update
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no API changes)
  • Build related changes
  • Other (please describe):

Testing

  • Did you test the pull request locally?
  • Did you add new tests?

How to test this PR?

  1. To test this PR you must have
    a. an installation of Nebari with jhub apps enabled
    b. at least 100 conda-store environments (or decrease the size_limit var in the PR to a sufficiently small number, like 2)
  2. Navigate to the nebari home screen
  3. Click on the deploy app button
  4. Click on the drop down to select the conda store environment for the app
  5. Notice that all the environments are available

Any other comments?

Note, that this API endpoint has the following known bug conda-incubator/conda-store#859.

Users may be able to get around this issue by refreshing the page to get the updated set of environments.

@soapy1
Copy link
Contributor Author

soapy1 commented Jan 14, 2025

I took a look around the code base to find where unit tests for this should go, but I could not find anything obvious. Would love some advice on where a good spot for a test would be. Thanks 🐎

@soapy1 soapy1 marked this pull request as ready for review January 14, 2025 18:13
@viniciusdc
Copy link
Contributor

viniciusdc commented Jan 20, 2025

I was following up on the discussion and had a meeting with Chuck as well; right now, nebari does not have a good architecture to test internal /stages Python code fully; it's basically in another realm than the Nebari package itself. Unless we did something in the direction of explicitly initializing each nested folder as a Python submodule, we can't easily import such methods.

I do like the overall idea and the sound of that, though this might be something to another PR, my suggestion would be to implement the pure method @dcmcand commented above for the URL string list generation and leave there for another PR where we make it possible to be unit-tested.

Regarding the discussion for the default value of PAGE_SIZE_LIMIT, I also agree that this should be overridable somehow; I would prefer this to be configurable somewhere, maybe via trait lets in the future, but right now, just allowing it to be changed by an environment variable might be enough.

@soapy1 soapy1 marked this pull request as draft January 20, 2025 18:58
@soapy1 soapy1 marked this pull request as ready for review January 20, 2025 21:49
@dcmcand
Copy link
Contributor

dcmcand commented Jan 22, 2025

@soapy1 we had a discussion about this at the Nebari community meeting and the consensus was for now, the simplest way forward would be to use importlib to import the filepath into the unit test file.

@soapy1
Copy link
Contributor Author

soapy1 commented Jan 23, 2025

the simplest way forward would be to use importlib to import the filepath into the unit test file.

Using importlib was also my first thought to resolving the testing issue. I commented earlier with a link to my test branch https://github.com/soapy1/nebari/compare/get-all-cs-env...soapy1:nebari:try-unit-test-spawner?expand=1. This approach is not really going to work (in a straight forward way) because the 02-spawner file is a traitlets config file and has all these c.Spawner.pre_spawn_hook = get_username_hook. There are probably some ways to get around this, but I would expect it will take a good amount of time for exploration

I do like the overall idea and the sound of that, though this might be something to another PR, my suggestion would be to implement the pure method @dcmcand commented above for the URL string list generation and leave there for another PR where we make it possible to be unit-tested.

IMO I think if tests are not getting added then it doesn't make sense to refactor things so that it might be easier to write tests in the future. For a couple reasons:

  1. The implementation of this function might need to change (for example to solve [ENH] - Filter all conda environments with given user permissions #2193, or to adopt cursor based pagination Sort envs returned by REST API by current build's scheduled_on time conda-incubator/conda-store#881). By changing the function now it might force unneeded restraints on future developments
  2. It would be better if whoever writes the test has agency to refactor things the way they think is best for however they have figured out how to run tests

@soapy1 soapy1 marked this pull request as draft January 23, 2025 18:48
@soapy1 soapy1 marked this pull request as ready for review January 23, 2025 21:47
@soapy1 soapy1 requested a review from dcmcand January 23, 2025 21:48
@soapy1
Copy link
Contributor Author

soapy1 commented Jan 23, 2025

Calling out that this PR uses the v1 api for environments which has this known bug conda-incubator/conda-store#859. v2 of the api implements cursor based pagination and resolves this issue. It will be available in the next release of conda-store.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: New 🚦
Development

Successfully merging this pull request may close these issues.

[BUG] - JHubApps Create App Page Selects only First ~100 Conda Environments
3 participants