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

Install python virtual environment with contributed ruamel.yaml #39

Merged
merged 11 commits into from
Jun 10, 2024

Conversation

awnawab
Copy link
Contributor

@awnawab awnawab commented May 6, 2024

This PR contributes the installation of a python virtual environment that contains the contributed fypp and ruamel.yaml modules. A minimal fckit_yaml_reader module is also installed that creates an API consistent with the pyyaml yaml.safe_load and yaml.dump functions. The aim is to provide yaml parsing functionality for offline builds of the ifs-bundle and ecwam-bundle. Downstream projects will therefore rely on either pyyaml+fypp or fckit.

I have tested the fckit_yaml_reader with ecwam, as a bundle build and as an externally built or installed project. I have also run the downstream_fypp tests by compiling with eckit.

@wdeconinck wdeconinck added the approved-for-ci Approved for CI run label May 6, 2024
@wdeconinck
Copy link
Member

It looks like we need to support CMake 3.18

set( Python3_FIND_VIRTUALENV STANDARD )
find_package( Python3 COMPONENTS Interpreter REQUIRED )

# Create a loki virtualenv
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# Create a loki virtualenv
# Create a virtualenv

@github-actions github-actions bot removed the approved-for-ci Approved for CI run label May 6, 2024
@awnawab
Copy link
Contributor Author

awnawab commented May 6, 2024

I've downgraded the minimum cmake to 3.17 and the minimum python to 3.6. This should fix the tests for everything except for the ubuntu gnu@ubuntu-22.04 tests. This runner seems to have a very barebones python installation:

  The virtual environment was not created successfully because ensurepip is not
  available.  On Debian/Ubuntu systems, you need to install the python3-venv
  package using the following command.
  
      apt install python3.10-venv

I think to fix this error we will have to update to the runner config.

@wdeconinck wdeconinck added approved-for-ci Approved for CI run and removed contributor labels May 7, 2024
@awnawab awnawab force-pushed the feature/yaml_reader branch from 93422d8 to 7ebddc8 Compare May 7, 2024 13:40
@github-actions github-actions bot removed the approved-for-ci Approved for CI run label May 7, 2024
@awnawab
Copy link
Contributor Author

awnawab commented May 7, 2024

As discussed offline, I've made the build and installation of the python virtual environment optional. This defaults to off, and if left disabled then we just install the fypp runner script like we used to.

The python virtual environment can only be enabled if python >= 3.8 is found. For most of the downstream tests this wasn't the case. The gnu@fedora-37 runners did however have a more up-to-date python installation. If possible, it would be great to configure these CI tests with the option -DENABLE_FCKIT_VENV=ON to have CI test coverage of the virtual environment.

@wdeconinck wdeconinck added the approved-for-ci Approved for CI run label May 7, 2024
Copy link
Member

@wdeconinck wdeconinck left a comment

Choose a reason for hiding this comment

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

Very nice improvements! Just a few minor edits requested.


# Create a virtualenv
set( VENV_PATH ${CMAKE_CURRENT_BINARY_DIR}/fckit_venv )
message( STATUS "Create Python virtual environment ${VENV_PATH}" )
Copy link
Member

Choose a reason for hiding this comment

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

Please use ecbuild_info() here and below for logging.

@@ -0,0 +1,68 @@
# (C) Copyright 2013 ECMWF.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# (C) Copyright 2013 ECMWF.
# (C) Copyright 2024 ECMWF.

@@ -0,0 +1,72 @@
#!/usr/bin/env python3

# (C) Copyright 2013 ECMWF.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# (C) Copyright 2013 ECMWF.
# (C) Copyright 2024 ECMWF.

@github-actions github-actions bot removed the approved-for-ci Approved for CI run label May 7, 2024
@wdeconinck wdeconinck added the approved-for-ci Approved for CI run label Jun 10, 2024
@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 50.77%. Comparing base (f44d3f9) to head (a18d4f0).
Report is 5 commits behind head on develop.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop      #39      +/-   ##
===========================================
- Coverage    53.34%   50.77%   -2.58%     
===========================================
  Files           54       55       +1     
  Lines         4508     4739     +231     
  Branches         0      450     +450     
===========================================
+ Hits          2405     2406       +1     
- Misses        2103     2333     +230     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@wdeconinck wdeconinck merged commit 1ec4c4c into ecmwf:develop Jun 10, 2024
141 of 142 checks passed
@awnawab awnawab deleted the feature/yaml_reader branch June 10, 2024 13:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved-for-ci Approved for CI run
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants