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

Refactor LAMBKIN #35

Merged
merged 15 commits into from
Apr 27, 2023
Merged

Refactor LAMBKIN #35

merged 15 commits into from
Apr 27, 2023

Conversation

hidmic
Copy link
Collaborator

@hidmic hidmic commented Apr 12, 2023

Proposed changes

Closes #17, #24, #34. Partially addresses #18 and #19. To call this a patch is an understatement. This is almost a different project. The overall architecture and spirit of LAMBKIN remains, but the implementation has changed substantially:

  • lambkin, the Python package, has been generalized and modularized to support further extension. A form of descriptive DSL within RobotFramework allows benchmark configuration, but many hooks are available for user override. Benchmark data access is now behind a Python API, enabling programmatic use (one step away from Jupyter Notebooks as a report #26).
  • Benchmark ROS packages have been migrated to match the new API. Overall structure remains the same.
  • Provisioning, and containerization in particular, has been reworked to be easier to develop and release packages, both in this repository and downstream, and to make better use of existing tooling instead of reinventing the wheel.

One notable omission are CI workflows. I intend to add them in an immediate follow-up PR.

Type of change

  • 🐛 Bugfix (change which fixes an issue)
  • 🚀 Feature (change which adds functionality)
  • 📚 Documentation (change which fixes or extends documentation)

💥 Breaking change! This a complete rework of this project. No way (and no need) to merge this incrementally while maintaining backwards compatibility.

Checklist

  • Lint and unit tests (if any) pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)

Additional comments

A good place to start looking into this is documentation (still scarce, I have plenty to write).

Each commit is a conceptually separate set of changes, which is not to say each commit is functional on its own. They are not. I abandoned that precept a long time ago. That said, I'm open to split this PR in whatever way makes it easier to review.

Signed-off-by: Michel Hidalgo <[email protected]>
Signed-off-by: Michel Hidalgo <[email protected]>
Signed-off-by: Michel Hidalgo <[email protected]>
Signed-off-by: Michel Hidalgo <[email protected]>
@hidmic
Copy link
Collaborator Author

hidmic commented Apr 12, 2023

One important question not yet answered above is why, why this change, why this huge PR. I must say I should've known better and I should've organized work so as to avoid a 10k LOC patch. To be honest, when I started with #35 I was not sure where I was heading. As the patch evolved, so did my understanding of the problem and the limitations of the existing implementation. Today, with this architecture and the mental model I have of it, LAMBKIN can sustain growth (of features, use cases, collaborators) in a way our humble proof of concept couldn't.

@hidmic
Copy link
Collaborator Author

hidmic commented Apr 12, 2023

FYI @agalbachicar @glpuga.

Copy link
Member

@nahueespinosa nahueespinosa left a comment

Choose a reason for hiding this comment

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

@hidmic This is more than amazing!

I'm going to review this by parts. Until now:

  • I followed the "getting started" guidelines to run local benchmarks (SLAM Toolbox Magazino and parameterized TUM).
  • I did a first pass on the general directory structure.
  • I did a first pass on the user-facing .robot scripts.

Copy link
Collaborator

@francocipollone francocipollone left a comment

Choose a reason for hiding this comment

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

Great job indeed,

I just did one pass. I will proceed to run different benchmarks with different options and see if I identify a bug or misbehavior.

CONTRIBUTING.md Show resolved Hide resolved
src/core/lambkin/src/lambkin/data/ros.py Outdated Show resolved Hide resolved
Copy link
Member

@nahueespinosa nahueespinosa left a comment

Choose a reason for hiding this comment

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

@hidmic LGTM! This is amazing.

I haven't gone through every single line of code, but I don't see any point in delaying this PR any longer.
Future documentation and feature work will reveal improvement points (if any), but this is as solid as it gets in my opinion.

@glpuga
Copy link
Collaborator

glpuga commented Apr 25, 2023

GorgeousNarrowFurseal-max-1mb

@hidmic hidmic merged commit e35ced6 into main Apr 27, 2023
@hidmic hidmic deleted the hidmic/refactor branch April 27, 2023 12:21
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.

Refactor lambkin core package
4 participants