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

Latest protobuf #126

Merged
merged 36 commits into from
Mar 20, 2023
Merged

Latest protobuf #126

merged 36 commits into from
Mar 20, 2023

Conversation

sverhoeven
Copy link
Member

@sverhoeven sverhoeven commented Jan 23, 2023

Refs #115

Adds Dockerfiles for heat model in combinations of languages, and versions of bmi, protobuf, grpc4bmi.

To test

  1. Build images with cd test/heat-images && docker compose build
  2. Edit image var in test/heat-images/heat_tester.py and run it

After merge actions

  • In test/heat-images/python-bmi02-legacy/Dockerfile, test/heat-images/python-bmi20/Dockerfile, test/heat-images/python-bmi20-pb4/Dockerfile, change RUN pip install https://github.com/eWaterCycle/grpc4bmi/archive/refs/heads/bmi2.zip to RUN pip install grpc4bmi==<new version>

@sverhoeven
Copy link
Member Author

@goord I am trying to create a grpc4bmi Docker image around https://github.com/csdms/bmi-example-cxx , but am having link errors. error message

Do you have any quick suggestions?

@sverhoeven sverhoeven changed the base branch from master to bmi2 January 26, 2023 14:59
@sverhoeven
Copy link
Member Author

In https://github.com/eWaterCycle/grpc4bmi/tree/latest-protobuf/test/heat-images/c-bmi20-pb3 the update and inialize methods work, but get_component_name core dumps.

Debug

Outside Docker do echo core | sudo tee /proc/sys/kernel/core_pattern + make ulimit -c does not return 0.

docker run -ti  --entrypoint bash heat:c-bmi20-pb3
apt-get update
apt-get install gdb
/usr/local/bin/run_bmi_server
# Call get_component_name in python shell
gdb /usr/local/bin/run_bmi_server /core.7
...
bt
#0  __strncpy_ssse3 () at ../sysdeps/x86_64/multiarch/strcpy-ssse3.S:98
#1  0x00007ff5bdd7b2a8 in Get_component_name () from /usr/local/lib/libbmiheatc.so
#2  0x00007ff5bde4cd59 in BmiCWrapper::GetComponentName[abi:cxx11]() () from /usr/local/lib/libgrpc4bmi.so
#3  0x00007ff5bde4dd4d in BmiGRPCService::getComponentName(grpc::ServerContext*, bmi::Empty const*, bmi::GetComponentNameResponse*) ()
...
import grpc
from grpc4bmi.bmi_grpc_client import BmiClient
model = BmiClient(grpc.insecure_channel("172.17.0.2:50051"))
model.get_component_name()
# raises
_InactiveRpcError: <_InactiveRpcError of RPC that terminated with:
	status = StatusCode.UNAVAILABLE
	details = "Socket closed"
	debug_error_string = "UNKNOWN:Error received from peer ipv4:172.17.0.2:50051 {grpc_message:"Socket closed", grpc_status:14, created_time:"2023-01-30T16:15:57.894638216+01:00"}"

@sverhoeven sverhoeven marked this pull request as ready for review February 21, 2023 11:43
@sverhoeven
Copy link
Member Author

Docker files are now hidden away in some sub dir (https://github.com/eWaterCycle/grpc4bmi/tree/latest-protobuf/test/heat-images). We should put them in better location, for example

  1. https://github.com/eWaterCycle/grpc4bmi-examples
  2. https://github.com/eWaterCycle/ewatercycle-models

What to you think @Peter9192 ?

@Peter9192
Copy link
Contributor

What to you think @Peter9192 ?

I thought we were going to retire grpc4bmi-examples in favour of ewatercycle-models. But perhaps we can instead repurpose it:

Does that make sense?

@sverhoeven sverhoeven requested a review from Peter9192 March 13, 2023 08:48
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.

Overall, very nice test suite! Got a few issues while testing. Also had some questions about the c++ changes, but I suppose those are not the most relevant, more to trigger myself to see if I could understand what was going on.

cpp/CMakeLists.txt Show resolved Hide resolved
cpp/CMakeLists.txt Show resolved Hide resolved
cpp/bmi_c_wrapper.h Show resolved Hide resolved
docs/server/Cpp.rst Outdated Show resolved Hide resolved
grpc4bmi/bmi_pb2.py Show resolved Hide resolved
test/heat-images/python-bmi20-pb4/Dockerfile Show resolved Hide resolved
image, config_body = (
# Comment out line you want to run
'heat:py-0.2',py_config
# 'heat:py-0.2-legacy',py_config
Copy link
Contributor

Choose a reason for hiding this comment

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

with grpc4bmi install from branch bmi2 this gives

(ewatercycle) peter@DESKTOP-S380217:~/ewatercycle/grpc4bmi/test/heat-images$ python heat_tester.py
heat:py-0.2-legacy
The 2D Heat Equation
('plate_surface__temperature',)
[6 8]
Traceback (most recent call last):
  File "/home/peter/ewatercycle/grpc4bmi/test/heat-images/heat_tester.py", line 59, in <module>
    test_heat(tmp_path=Path('/tmp'))
  File "/home/peter/ewatercycle/grpc4bmi/test/heat-images/heat_tester.py", line 53, in test_heat
    values = model.get_value(var_name)
  File "/home/peter/mambaforge/envs/ewatercycle/lib/python3.10/site-packages/grpc4bmi/bmi_optionaldest.py", line 105, in get_value
    dest = reserve_values(self.origin, name)
  File "/home/peter/mambaforge/envs/ewatercycle/lib/python3.10/site-packages/grpc4bmi/reserve.py", line 12, in reserve_values
    size = total_size // item_size
ZeroDivisionError: integer division or modulo by zero

test/heat-images/docker-compose.yaml Outdated Show resolved Hide resolved

image, config_body = (
# Comment out line you want to run
'heat:py-0.2',py_config
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
'heat:py-0.2',py_config
'heat:python-bmi02',py_config

With bmi2 branch insides my existing ewatercycle env gives zerodivisionerror:

heat:python-bmi02
The 2D Heat Equation
('plate_surface__temperature',)
[6 8]
Traceback (most recent call last):
  File "/home/peter/ewatercycle/grpc4bmi/test/heat-images/heat_tester.py", line 59, in <module>
    test_heat(tmp_path=Path('/tmp'))
  File "/home/peter/ewatercycle/grpc4bmi/test/heat-images/heat_tester.py", line 53, in test_heat
    values = model.get_value(var_name)
  File "/home/peter/mambaforge/envs/ewatercycle/lib/python3.10/site-packages/grpc4bmi/bmi_optionaldest.py", line 105, in get_value
    dest = reserve_values(self.origin, name)
  File "/home/peter/mambaforge/envs/ewatercycle/lib/python3.10/site-packages/grpc4bmi/reserve.py", line 12, in reserve_values
    size = total_size // item_size
ZeroDivisionError: integer division or modulo by zero

Copy link
Contributor

Choose a reason for hiding this comment

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

With clean mamba env and grpc4bmi installed from bmi2 branch, get

Traceback (most recent call last):
  File "/home/peter/ewatercycle/grpc4bmi/test/heat-images/heat_tester.py", line 4, in <module>
    from grpc4bmi.bmi_client_docker import BmiClientDocker
  File "/home/peter/mambaforge/envs/testheat/lib/python3.10/site-packages/grpc4bmi/bmi_client_docker.py", line 8, in <module>
    from typeguard import check_argument_types, qualified_name
ImportError: cannot import name 'check_argument_types' from 'typeguard' (/home/peter/mambaforge/envs/testheat/lib/python3.10/site-packages/typeguard/__init__.py)

downgrading typeguard from v3.0.0 to v2.13.3 solved this issue.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed in #129

Copy link
Member Author

Choose a reason for hiding this comment

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

I will have a look at the division by zero as the workaround in bmi_grpc_client.py::get_var_itemsize should fix it

Copy link
Member Author

Choose a reason for hiding this comment

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

That division by zero workaround is only in branch of this PR at https://github.com/eWaterCycle/grpc4bmi/blob/latest-protobuf/grpc4bmi/bmi_grpc_client.py#L199 so that makes sense then when running the bmi2 branch it fails.

pyproject.toml Outdated Show resolved Hide resolved
Copy link
Member Author

@sverhoeven sverhoeven left a comment

Choose a reason for hiding this comment

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

Thanks for reviewing

cpp/CMakeLists.txt Show resolved Hide resolved
cpp/CMakeLists.txt Show resolved Hide resolved
cpp/bmi_c_wrapper.h Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
test/heat-images/c-bmi20/Dockerfile Show resolved Hide resolved
test/heat-images/cxx-bmi20/Dockerfile Show resolved Hide resolved

image, config_body = (
# Comment out line you want to run
'heat:py-0.2',py_config
Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed in #129


image, config_body = (
# Comment out line you want to run
'heat:py-0.2',py_config
Copy link
Member Author

Choose a reason for hiding this comment

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

I will have a look at the division by zero as the workaround in bmi_grpc_client.py::get_var_itemsize should fix it

test/heat-images/python-bmi20-pb4/Dockerfile Show resolved Hide resolved
Copy link
Member Author

@sverhoeven sverhoeven left a comment

Choose a reason for hiding this comment

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

Thanks for reviewing

test/heat-images/docker-compose.yaml Outdated Show resolved Hide resolved

image, config_body = (
# Comment out line you want to run
'heat:py-0.2',py_config
Copy link
Member Author

Choose a reason for hiding this comment

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

That division by zero workaround is only in branch of this PR at https://github.com/eWaterCycle/grpc4bmi/blob/latest-protobuf/grpc4bmi/bmi_grpc_client.py#L199 so that makes sense then when running the bmi2 branch it fails.

@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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants