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.

Sign up for GitHub

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

Jump to bottom

Combine 0.2.x branch with master #124

Merged
merged 178 commits into from
Mar 20, 2023
Merged

Combine 0.2.x branch with master #124

merged 178 commits into from
Mar 20, 2023

Conversation

sverhoeven
Copy link
Member

@sverhoeven sverhoeven commented Jan 5, 2023 •
edited
Loading

The 0.2.x branch:

  • has been used the most.
  • uses old 0.2 bmi spec
    The master branch
  • has not been used a lot
  • use current 2.0 bmi spec

This PR combines the v0.2.x branch with master.

Also

  • switches from setup.py+requirements.txt 2 pyproject.toml
  • switches to 1 dimensional BmiHeat, was 2d before
  • drops unused dev dependencies
  • Adds OptionalDestBmi class with get_value(name,dest=None)
  • Docs for branch at https://grpc4bmi.readthedocs.io/en/bmi2/

sverhoeven and others added 30 commits February 19, 2020 09:49
@sverhoeven
Merge pull request #58 from eWaterCycle/singularity-from-notebook-56-…
35ebc87 
…bmiport-envvar-55

Singularity from notebook 56 bmiport envvar 55
@sverhoeven
Prep for 0.2.4 release
12d7414
@sverhoeven
Attempt to create grpc channel on occupied port (#64)
1ef38b6 
* Added tests for create_grpc_channel

Refs #63

* Dont track ports used by client

The are not released and the client should not care if the same port is used multiple times.

Refs #63

* Added timeout argument to Docker, Singularity and Subprocess clients.

Refs #63
@sverhoeven
Prep for v0.2.5 release
2a1cc40
@sverhoeven
Prep for 0.2.6 release
e73ed9e 
Created GitHub release from wrong branch
@sverhoeven
Added extra_volumes arg to BmiClientDocker
f7f422f 
By manual cherry-pick of 6661451
@sverhoeven
Added extra_volumes arg to BmiClientSingularity
479a848 
By merging changes in https://github.com/eWaterCycle/grpc4bmi/pull/69/files into 0.2.x branch
@sverhoeven
Merge pull request #71 from eWaterCycle/extra-volumes-0.2.0
82e47a4 
Extra volumes 0.2.x
@sverhoeven
Prep for 0.2.7 release
24eccec
@sverhoeven
Backport 4ca0fe3
cb639df 
Refs #74
@sverhoeven
Added test to trigger RESOURCE_EXHAUSTED error
9ef3a2b
@sverhoeven
Implemented chunked get_value
b7f1ec2 
Refs #76
@sverhoeven
Swap travis for GH action
8c2a915
@sverhoeven
Revert "Swap travis for GH action"
51dd5ae 
This reverts commit 8c2a915.
@sverhoeven
Swap travis for GH action
3e05977
@sverhoeven
Build & install bmi-c
314fd54
@sverhoeven
Added sonar-project.properties
48d4a32
@sverhoeven
Merge branch '0.2-gh-action' into value-too-big-0.2.x
c285914
@sverhoeven
No debug cli arg in 0.2.x branch
9189b46
@sverhoeven
Revert "No debug cli arg in 0.2.x branch"
d13ddec 
This reverts commit 9189b46.
@sverhoeven
No debug cli arg in 0.2.x branch, take 2
65d985d
@sverhoeven
Merge pull request #78 from eWaterCycle/value-too-big-0.2.x
2d00924 
0.2.x: Handle when model value is too big for message
@sverhoeven
Prep for 0.2.8 release
15a82db
@sverhoeven
Added sleep to BmiClientSingularity
c1460b7 
Fixes #84
@sverhoeven
Merge pull request #85 from eWaterCycle/sing-sleep-84
f425f68 
Added sleep to BmiClientSingularity
@sverhoeven
Prep for 0.2.9
1ad1969
@sverhoeven
Replace parent env with injected env var
2d49220 
Seems singularity version on cartesius.surfsara.nl no longer accepts parent env.

Fixes #84
@sverhoeven
Prep for 0.2.10 release
fe89680
@sverhoeven
Raise required singularity version so --env works.
7480d73
@sverhoeven
Use sing v.3.6.4
98d7d8a
@sverhoeven sverhoeven marked this pull request as ready for review February 21, 2023 11:43
@sverhoeven sverhoeven mentioned this pull request Mar 7, 2023
Closed
5 tasks
@sverhoeven sverhoeven requested a review from Peter9192 March 13, 2023 08:48
Peter9192
Peter9192 approved these changes Mar 15, 2023
View reviewed changes
Copy link
Contributor

@Peter9192 Peter9192 left a comment

Choose a reason for hiding this comment

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

Great to merge these diverging branches! Since most of the changes have already been reviewed separately, in this review I tried to focus on how this merge affects the user experience.

Tried locally with example marrmot case, with apptainer, docker, and through ewatercycle. All work. It would be helpful to have a couple of example calls in the documentation that work out of the box by copy-pasting in https://grpc4bmi.readthedocs.io/en/bmi2/api/grpc4bmi.bmi_client_docker.html. I was also able to successfully run the ewatercycle test suite with this branch (using eWaterCycle/ewatercycle#339).

How do we envision future development? Will we only maintain the main branch from now on? I think it might be helpful to write something like a changelog entry about this merge, and what it means for legacy bmi containers.

.readthedocs.yaml Show resolved Hide resolved
README.md Show resolved Hide resolved
docs/conf.py Show resolved Hide resolved
docs/server/python.rst Outdated Show resolved Hide resolved
docs/server/python.rst Outdated
$ run-bmi-server --bmi-version 0.2 --name mypackage.mymodule.MyLegacyBmi

The functions only present in v0.2 will not be callable.
Copy link
Contributor

Choose a reason for hiding this comment

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

So this means that people that were installing grpc4bmi with the v0.2 bmi will loose this functionality when they change over to the new master branch release. Should we somehow provide update instructions for them?

One option is to make a last release of the v0.2 branch with a message that this will be the last release and people are encouraged to update to the latest version of grpc4bmi and remove the deprecated bmi calls in their models

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, is there a diff somewhere to see which functions differ between bmi v2 and v0.2?

grpc4bmi/bmi_optionaldest.py Outdated Show resolved Hide resolved
grpc4bmi/bmi_optionaldest.py Outdated Show resolved Hide resolved
grpc4bmi/bmi_optionaldest.py Show resolved Hide resolved
pyproject.toml Show resolved Hide resolved
test/test_client.py Outdated Show resolved Hide resolved
@sverhoeven
Copy link
Member Author

  1. I have enabled PR check in readthedocs
  2. Added section container/usage.html that legacy container image can be used and expanded server/python page to list added/removed functions from BMI.

@sonarqubecloud
Copy link

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

99.0% 99.0% Coverage
3.3% 3.3% Duplication

@sverhoeven sverhoeven merged commit 236ec61 into master Mar 20, 2023
@sverhoeven sverhoeven deleted the bmi2 branch March 20, 2023 13:40
@sverhoeven sverhoeven mentioned this pull request Mar 20, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Peter9192 Peter9192 Peter9192 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@sverhoeven @Peter9192