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

Improve workflows #353

Merged
merged 115 commits into from
Dec 30, 2022
Merged

Improve workflows #353

merged 115 commits into from
Dec 30, 2022

Conversation

ad-daniel
Copy link
Collaborator

@ad-daniel ad-daniel commented Nov 23, 2022

Improves the workflow and fixes issues

@ad-daniel ad-daniel added the enhancement New feature or request label Nov 23, 2022
@ad-daniel ad-daniel self-assigned this Nov 23, 2022
@ad-daniel ad-daniel added the test release Tests if a wheel created from a branch runs correctly label Nov 23, 2022
@ad-daniel ad-daniel added the test sources Run style checks label Nov 25, 2022
@ad-daniel ad-daniel added the test tools Test the toolkit methods label Dec 28, 2022
@ad-daniel ad-daniel added test sources Run style checks and removed test sources Run style checks labels Dec 28, 2022
@ad-daniel
Copy link
Collaborator Author

This PR:

  • Splits skeleton tests to avoid running out of RAM
  • Deactivates object detection 3d test. It works in all cases, namely: locally, with test tools (installed from repo) and test release (installed from docker & wheel). However in the wheel & docker cases although the test pass, unittest ends up crashing just before finishing due to an invalid free().
    I've looked into it a bit, and just having the import from opendr.perception.object_detection_3d import VoxelObjectDetection3DLearner suffices for it to fail this way. What's wrong with it to cause the illegal free() I don't know
  • Reworks the test release workflow so that only engine + tool are installed.
    • This was already the case before, but tools that rely on other tools were installed across the board (in all test conditions), which was hiding some missing dependencies.
    • The introduced limits in the dependencies (like protobuf) are necessary as the latest version of this package results in a failure. This dependency is often installed indirectly (onnx, tensorboard, tensorboardx), and these are often unconstrained which results in failures if the tools are installed separately and no indirect limits are present
  • I've upgraded the version to 2.0.0 already because I had the feeling the test release builds ended up installing stuff from pypi (since a version 1.1.1 is present online) rather than the tar.gz files downloaded from the artifact. After doing so, an "issue" emerged with tools relying on object detection 2d wheel (directly or indirectly), basically they all failed if the tool was installed from pip and the reason is that they rely on cythonized modules which, when installing the tool from pip, doesn't appear to include the dynamic libraries (*.so) and therefore couldn't be imported. The extra modules are indeed included here when building the wheel, however as far as I can tell they are not generated automatically on a pip install but only if installed with python3 setup.py build_ext --in-place. This is the reason for the addition of the line cd src/opendr/perception/object_detection_2d/retinaface; make; cd $OPENDR_HOME in the workflow for test release so that they are generated (users would need to unpack the tar.gz and do it manually). It works but honestly I don't like it. @passalis, do you know of any other way this could be done, or ideally for it to occur automatically when installing from pip? I have the feeling there must be a way since in the previous version of the workflow this wasn't necessary. So we either introduced a bug in develop which was hidden by the fact that test release was downloading stuff from pypi, or it never had the chance of working from a pip installation. Either way it stinks that it only shows now, fundamentally the workflow job is the same, just more strict. If they aren't generated now, they shouldn't before either which means it should've failed long before, irrespective of where it gets the wheel from.

Finally I've spent quite some time trying to get mobile manipulation, single demo grasp and end-2-end planning wheel packages to work separately (the installation from repo works just fine). For them to work significant restructuring is necessary and I think they need a bash script or something that sets up their environment in case they're installed separately and this method needs to play nice with the existing installation pipeline (where some stuff is installed from .inis, some from install.sh, they themselves have additional requirements (ex: install_mobile_manipulation), etc.). I tried for the fun of it, but I've encountered a ton of dependency problems.
To be frank the more I looked into it, the more I'm not sure users would want to install it this way. Of course it's nice not needing to install the entire toolkit for users to use these tools, but I'm not sure pip is the way to go considering these have rather special requirements. They would need to run a bash script anyway to install the rest of the stuff.

I don't think it's wise or necessary to do this now (especially since single demo grasp will be reworked), but we need to decide whether we should still include these 3 tools in the packages.txt (and therefore generate wheels for them) or if instead adding them there at a later date if indeed we want to go down this route.

@ad-daniel ad-daniel marked this pull request as ready for review December 29, 2022 10:14
Copy link
Collaborator

@tsampazk tsampazk left a comment

Choose a reason for hiding this comment

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

Thank you very much Daniel, improvements look great.

I just noticed that protobuf is listed twice in two dependencies files, probably by mistake?

ad-daniel and others added 2 commits December 29, 2022 19:56
Co-authored-by: Kostas Tsampazis <27914645+tsampazk@users.noreply.github.com>
…on_3d/dependencies.ini

Co-authored-by: Kostas Tsampazis <27914645+tsampazk@users.noreply.github.com>
tsampazk
tsampazk previously approved these changes Dec 29, 2022
Copy link
Collaborator

@tsampazk tsampazk left a comment

Choose a reason for hiding this comment

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

LGTM, thanks! 🚀

@passalis
Copy link
Collaborator

Thank you Daniel!!! I agree with you regarding the mobile manipulation, single demo grasp and end-2-end planning.These most probably are not going to be used directly from pip packages. Regarding the building of the cythonized extension, I remember that I had found a way to fix this in a previous release. Give me a few hours and I will let you know by tomorrow the latest If I have found a solution.

Dirty commit, comments needs to removed.
@passalis
Copy link
Collaborator

Hi @ad-daniel, I am directly changed a few lines to test something - I hope this is fine. I didn't manage to reproduce the issue locally just by installing the package. So, there might be something related to the test setup.

One possibility is that setting PYTHONPATH overrides the package installed by pip, since we are re-exposing the toolkit source to the python interpreter. Otherwise, I cannot understand why building the extensions after installing the packages helps.

Copy link
Collaborator

@passalis passalis left a comment

Choose a reason for hiding this comment

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

I removed the manual make commands. This seems to be working, if I didn't miss anything.

@ad-daniel
Copy link
Collaborator Author

I see, I was testing other stuff at the same time, so that is indeed very plausible. Likewise, it wasn't present in the previous version so that could very well be the culprit. Thanks a lot!

@ad-daniel ad-daniel merged commit 73660ce into develop Dec 30, 2022
@ad-daniel ad-daniel deleted the improve-workflows branch December 30, 2022 10:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request test release Tests if a wheel created from a branch runs correctly test sources Run style checks test tools Test the toolkit methods
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants