Skip to content

Conversation

@bouweandela
Copy link
Member

@bouweandela bouweandela commented Dec 9, 2022

Description

Avoid issues due to importing private functions from ESMValCore. Related to ESMValGroup/ESMValCore#1835.

Closes #2973


Before you get started

Checklist

It is the responsibility of the author to make sure the pull request is ready to review. The icons indicate whether the item will be subject to the 🛠 Technical or 🧪 Scientific review.

New or updated recipe/diagnostic


To help with the number of pull requests:

@bouweandela bouweandela force-pushed the update-data-finder-imports branch from e0bbf84 to 46ab609 Compare December 9, 2022 15:46
@bouweandela
Copy link
Member Author

The tests are failing due to #2959

@bouweandela bouweandela marked this pull request as ready for review December 9, 2022 19:40
Copy link
Contributor

@remi-kazeroni remi-kazeroni left a comment

Choose a reason for hiding this comment

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

Thanks @bouweandela! That looks good to me. Just one question: wouldn't it be needed to update environment_osx.yml as well?

@bouweandela
Copy link
Member Author

Well spotted! Updated in 69e72c8

@bouweandela
Copy link
Member Author

bouweandela commented Dec 15, 2022

@remi-kazeroni Is there anything left to do on this pull request? The test failures appear unrelated, but I will try merging the main branch to see if it solves them.

@remi-kazeroni
Copy link
Contributor

@remi-kazeroni Is there anything left to do on this pull request? The test failures appear unrelated, but I will try merging the main branch to see if it solves them.

No, the PR looks ready to me. I was hesitant to click the approval button because of the test failures. Let's see what we get with the latest merge of main.

@bouweandela
Copy link
Member Author

@remi-kazeroni All green now!

Copy link
Contributor

@remi-kazeroni remi-kazeroni left a comment

Choose a reason for hiding this comment

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

Thanks @bouweandela, the PR looks good to me! @valeriupredoi, I leave it to you to do a final check and merge 🍻

@valeriupredoi
Copy link
Contributor

GA tests are failing from test recipe filler, @bouweandela you wanna have a look or should I (since that's my code); I can in a little while

@bouweandela
Copy link
Member Author

bouweandela commented Dec 16, 2022

It would be great if you could have a look at that in a new pull request. The recipe filler tool may not be that relevant anymore if we manage to include ESMValGroup/ESMValCore#1609 in the next release, but the deadline is getting near and we're not there yet, so it may be worth it to keep the recipe filler around for another release (or two to give people some time to transition).

@valeriupredoi
Copy link
Contributor

Sounds good buds, am on the bus now, as soon as I get in work will remove the GA test call, approve and merge this, then open an issue and fix those tests in a PR 👍

Copy link
Contributor

@valeriupredoi valeriupredoi left a comment

Choose a reason for hiding this comment

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

nice @bouweandela - we are still importing private bits though ie _config 😉

@valeriupredoi
Copy link
Contributor

issue for recipe_filler failed tests #2975 - I shalls fix 🎄

@valeriupredoi valeriupredoi merged commit 9d3a65b into main Dec 16, 2022
@valeriupredoi valeriupredoi deleted the update-data-finder-imports branch December 16, 2022 13:13
@valeriupredoi
Copy link
Contributor

great work @bouweandela and @remi-kazeroni 🎄

@bouweandela
Copy link
Member Author

@bouweandela - we are still importing private bits though ie _config 😉

I know, this pull request was about fixing the problems caused by importing from esmvalcore._data_finder. There is another one for importing from esmvalcore._config here #2736. I wish people stopped doing this and reviewers would start telling people not to do it.. this issue keeps coming back again and again.

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.

Tests fail in full development mode (esmvalcore and esmvaltool installed from source) due to _data_finder module deprecation

4 participants