mamba-org / gator

Conda environment and package management extension from within Jupyter
Other
260 stars 30 forks source link

Speed up list_available #133

Closed ericpre closed 3 years ago

ericpre commented 3 years ago

As discussed in https://github.com/mamba-org/gator/issues/132 and https://github.com/mamba-org/mamba/issues/835, use mamba repoquery search "*" --json in favour of mamba search --json (or conda search --json), since the later is significantly slower than mamba repoquery.

github-actions[bot] commented 3 years ago

Binder :point_left: Launch a binder notebook on the branch _ericpre/gator/speed_up_get_listavailable

ericpre commented 3 years ago

The test seem to be playing up, no sure if this is related to PR, because running the test suite on the master isn't working - see for example https://github.com/ericpre/gator/actions/runs/722378324.

wolfv commented 3 years ago

Cool, thanks for working on this!

fcollonval commented 3 years ago

Hey @ericpre

thanks for the PR. I carried out some performance tests (absolute figures donot matter much here):

image

Your solution will definitely improve experienced for mamba users.

@wolfv I'm not familiar with the mamba details. Does mamba repoquery search update the packages cache if it is expired?

ericpre commented 3 years ago

Thanks @ericpre for this PR. I left a few improvements.

Could you also add a test for it?

For that the best would be to check the call of f in

https://github.com/ericpre/gator/blob/3b9c1845f2fed9c8e2997ecba9dabcc560ad29d0/mamba_gator/tests/test_api.py#L794

You could test the call with something like:

f.assert_called_with(...)

after line 975. Note the call arguments will depend on the manager.

Sorry, I am not sure that I understand, what should be tested exactly? Do you want to have the ouptut of self._execute(self.manager, "repoquery", "search", "*", "--json") tested? list_available is already tested and the changes of this PR should be covered by these tests?

fcollonval commented 3 years ago

Sorry, I am not sure that I understand, what should be tested exactly? Do you want to have the ouptut of self._execute(self.manager, "repoquery", "search", "*", "--json") tested? list_available is already tested and the changes of this PR should be covered by these tests?

As you said, the output is already tested. I would like only to add a test to be sure the proper command is invoked depending on the manager; i.e. if conda, _execute should be called with ["conda", "search", "--json"] - if mamba, it should be called with [manager, "repoquery", "search", "*", "--json"].

Does it clarify the intention?

ericpre commented 3 years ago

Yes, it makes sense, thanks! It seems that the tests need to be adapted to pass the changes of this PR and since I don't understand how these tests works, it would take me quite a bit time to get it to work and I will able to do this here. Can you please take over the PR?

fcollonval commented 3 years ago

Thanks @ericpre I added the tests.

ericpre commented 3 years ago

Thanks for sorting out the test and finishing the PR!