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

Workaround for duplicate models for pit_crew #492

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

methylDragon
Copy link
Collaborator

@methylDragon methylDragon commented Apr 4, 2024

Although the invariant for the fuel database where a (model_name-author-name) pair is meant to be unique, a duplicate model still slipped through. This causes pit_crew to halt early, causing an incomplete model cache from being built, at worst missing all models on fuel dated earlier than the duplication when building the model cache from scratch.

This causes any fresh builds of RMF demos (and any other building yamls for that matter) using those missing models to fail to find and download them.

The Fix

This uses a triplet of (model_name-author_name-upload_time) to distinguish models, instead of (model_name-author_name).

GIANT NOTE

This is not tested against someone running pit_crew's download against a pre-existing model cache. I also haven't refined this yet. I'm just uploading this to present the idea.

This is also a little messy, since we also used the ModelNames named-tuple to fetch and classify models from the local cache AND the local gazebo model directory. This is problematic because those don't have upload times, so there's a data mismatch.

@methylDragon methylDragon marked this pull request as draft April 4, 2024 09:51
@methylDragon methylDragon force-pushed the ch3/workaround-duplicate-model branch from 9913a37 to a9ab88e Compare April 4, 2024 10:07
@methylDragon methylDragon force-pushed the ch3/workaround-duplicate-model branch from a9ab88e to efa3ace Compare April 4, 2024 10:07
@koonpeng
Copy link
Contributor

koonpeng commented Apr 5, 2024

This addresses the same issue as #490 but in a more complete way. The best approach is still fixing fuel's db, if we can do that then both #490 and this can be closed.

@luca-della-vedova
Copy link
Member

Merged #490 for a hotfix but reverted it when merging main back to this. I'll start testing it now on a prepopulated cache and see how that works

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.

3 participants