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

COPDS-1531: beta api #95

Merged
merged 17 commits into from
Apr 24, 2024
Merged

COPDS-1531: beta api #95

merged 17 commits into from
Apr 24, 2024

Conversation

malmans2
Copy link
Contributor

No description provided.

Copy link
Collaborator

@gbiavati gbiavati left a comment

Choose a reason for hiding this comment

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

Overall the pull requests can be approved after changing or removing the url and token.
If the integration test is needed it should use a GitHub secret

),
(
"http://cds2-test.copernicus-climate.eu/api",
"00000000-0000-4000-a000-000000000000",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe this test should use secrets

Copy link
Contributor Author

@malmans2 malmans2 Feb 29, 2024

Choose a reason for hiding this comment

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

Fine by me, but we (B-Open) do not have the permissions to add GH secrets.
Also, GH secrets would not work when pytest is executed locally.

@malmans2
Copy link
Contributor Author

Overall the pull requests can be approved after changing or removing the url and token. If the integration test is needed it should use a GitHub secret

Done. The new test checks whether the class is properly instantiated, so no secret is needed.

@gbiavati
Copy link
Collaborator

gbiavati commented Mar 5, 2024

Going from old api to new api we need the possibility to create integration tests.
Several keyword arguments of the current client are not implemented in the new version.
Some of those are essential for producing tests suites, like retry_max.
Please implement it asap.

Copy link
Collaborator

@gbiavati gbiavati left a comment

Choose a reason for hiding this comment

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

I will check tomorrow.

@malmans2
Copy link
Contributor Author

malmans2 commented Mar 6, 2024

Hi @gbiavati,

I can not reproduce the installation issue you reported last night.

Going from old api to new api we need the possibility to create integration tests. Several keyword arguments of the current client are not implemented in the new version. Some of those are essential for producing tests suites, like retry_max. Please implement it asap.

retry_max, sleep_max, and timeout are implemented in cads-api-client==0.8.2

Here is a minimal snippet to install the latest versions and download a dummy file.

conda create -n test-cdsapi-beta -c conda-forge pip --yes
conda activate test-cdsapi-beta
pip install git+https://github.com/bopen/cdsapi.git@COPDS-1531-beta-api
export CDSAPI_RC=$HOME/.cadsapirc
python -c "import cdsapi; client = cdsapi.Client(sleep_max=120, retry_max=500, timeout=60); client.retrieve('test-adaptor-dummy', {}, 'test.grib')"

@gbiavati
Copy link
Collaborator

gbiavati commented Apr 18, 2024

Hi @malmans2 ,
The installation of this api using pip may produce different results if a user has a clean environment or an existing installation of the cdsapi.
One reason can be the version defined in

version = "0.6.1"

it is the same of the current api, so it would not override/update normally.

This package is meant to facilitate the migration to the new system, so its installation and usage should be tested more as an upgrade of existing environment, not as a fresh install

@malmans2
Copy link
Contributor Author

Hi @gbiavati,

As cdsapi does not infer the version from git tags, I believe that the hardcoded variable version is manually updated by cdsapi maintainers when they issue a new release. Therefore, when this PR will be merged and a new release will be issued, users will be able to easily upgrade to the latest version with one of the following commands:

pip install --upgrade cdsapi
conda update -c conda-forge cdsapi

We added all dependencies in both cdsapi and cads-api-client, so users can just upgrade cdsapi without having to install any additional dependency.
See:

@malmans2
Copy link
Contributor Author

If you need to install this PR in an existing environment, this command worked fine for me:

pip install --upgrade --force-reinstall git+https://github.com/bopen/cdsapi.git@COPDS-1531-beta-api

Copy link
Collaborator

@gbiavati gbiavati left a comment

Choose a reason for hiding this comment

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

The API should be released

@EddyCMWF EddyCMWF merged commit 7c31b0f into ecmwf:master Apr 24, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants