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

Add CI/CD workflow #2

Merged
merged 8 commits into from
Oct 26, 2020
Merged

Add CI/CD workflow #2

merged 8 commits into from
Oct 26, 2020

Conversation

diegoferigo
Copy link
Member

Closes #1.

This PR add a new GitHub workflow to package, install, test, and publish to PyPI the sdist package. This time I am experimenting PEP517 and PEP518 support.

Few resources:

@diegoferigo diegoferigo self-assigned this Aug 24, 2020
@diegoferigo
Copy link
Member Author

After the upstream fix robotology-legacy/gym-ignition#234, the sdist gets compiled successfully, but the test fails when runs in the workflow. Locally instead it works fine.

@diegoferigo
Copy link
Member Author

diegoferigo commented Aug 24, 2020

I managed to reproduce the failure in a local docker image:

[libprotobuf ERROR google/protobuf/descriptor_database.cc:57] File already exists in database: peer_info.proto
[libprotobuf FATAL google/protobuf/descriptor.cc:1164] CHECK failed: generated_database_->Add(encoded_file_descriptor, size): 
terminate called after throwing an instance of 'google::protobuf::FatalException'
  what():  CHECK failed: generated_database_->Add(encoded_file_descriptor, size):

Yet another time, problems with protobuf 😑 I think that the problem is that the gym-ignition-nightly wheel is built and linked against a version of protobuf installed from sources. Instead, here, since we install Ignition Robotics from the repo, the regular protobuf version is loaded.

I'm almost done with robotology-legacy/gym-ignition#230, and in Focal it seems we no longer need to use a custom protobuf version for tensorflow compatibility (just the import order in Python is important). I'll leave this PR pending waiting to update upstream to Focal.


I think that also the problem we experienced in robotology-legacy/gym-ignition#76 was the same as here.

@diegoferigo diegoferigo force-pushed the feature/ci_cd branch 4 times, most recently from 5d9b4f4 to 404e1e9 Compare August 31, 2020 22:27
@diegoferigo
Copy link
Member Author

diegoferigo commented Sep 1, 2020

I did some tests but even after packaging gym-ignition-nightly from Focal, the same error occurs. I start suspecting that the problem is not (only) related to the protobuf version that should match in the packaging (CI) and user systems. When we package the wheel, we link against a statically-compiled version of ign-gazebo. This is due to our temporary need to use a custom version of the simulator while few upstream changes will get merged. I fear that this process embeds in the resulting library also libprotobuf.a.

This is totally ok if no other libraries try to load another version of protobuf. Unfortunately, this is the case of plugin libraries like those provided by this repo. In fact, the error reported above occurs not when scenario is loaded, but when the plugin is loaded during runtime. The OS tries to load libprotobuf.so from the file system, but maybe it collides with already loaded symbols contained by the embedded libprotobuf.a. cc @traversaro

I don't have many ideas, I tried to tweak a bit the dlopen flags from Python but I didn't find any working configuration. At this point I will leave this PR pending (again) waiting that our changes land upstream.

@traversaro
Copy link
Contributor

Whoah, the situation feels intricated. Yes, probably waiting for switching everything to Focal make sense. More in general, this show the shortcoming of using a distribution system that does not deal explicitly with binary C/C++ dependencies. In the long term, using something like conda could be beneficial to deal with this problems. However, I am afraid that on Python there is no silver bullet, especially when dealing with Tensorflow, as for example Tensorflow 2 is still not packaged for conda-forge, see conda-forge/tensorflow-feedstock#70 .

@diegoferigo
Copy link
Member Author

I am still hesitant to switch to systems like conda-forge because it could be problematic each time we need to use a package that is not yet part of it (e.g. ray-project/ray#585). It's interesting and worth keeping an eye on it, but I suspect that it's not as mature as we require (and note we're quite prone to use modern technologies like focal / python3.8 / C++17, ...).

@diegoferigo diegoferigo merged commit 5d20605 into master Oct 26, 2020
@diegoferigo diegoferigo deleted the feature/ci_cd branch October 26, 2020 09:54
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.

Add a Github workflow to automatically push the package to PyPI
2 participants