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 'description' to unassigned #403

Closed
ghost opened this issue Nov 11, 2020 · 8 comments · Fixed by #627
Closed

add 'description' to unassigned #403

ghost opened this issue Nov 11, 2020 · 8 comments · Fixed by #627

Comments

@ghost
Copy link

ghost commented Nov 11, 2020

First and foremost I'd like to thank you for all your work on vroom project!

Currently for jobs and shipments solutions, steps contains their descriptions -- this is very useful for programatically tracking their initial locations (e.g. we add our internal ID's there.)

However if the solution is not found, they are added to unassigned array without their corresponding descriptions. This makes it harder for us to figure out their initial locations (have to indirectly go through initial json input).

I think it would be very useful if unassigned stops had their initial descriptions. Also similarly for start and end steps (would be great if those had their vehicle startDescription and endDescription). Thanks!

@jcoupey
Copy link
Collaborator

jcoupey commented Nov 11, 2020

we add our internal ID's there

What about simply using your internal values as job/pickup/delivery id? Then you'd get the values you're looking for directly under id in the unassigned array.

@ghost
Copy link
Author

ghost commented Nov 12, 2020

What about simply using your internal values as job/pickup/delivery id? Then you'd get the values you're looking for directly under id in the unassigned array.

That was actually our first revision! :) However we found out that multiple jobs/shipments can have the same points -- and for VROOM all job/shipment IDs have to be unique. So we auto-generate them, and store our internal ID's in descriptions.

@jcoupey
Copy link
Collaborator

jcoupey commented Nov 12, 2020

for VROOM all job/shipment IDs have to be unique

Actually no, currently you can have duplicate ids across task types (job, pickup and delivery) and even for the same task type. This may change in the future as it would make sense to force unique (id,type) pairs.

I don't really get what's blocking you from using your own ids in id values, can you share a minimum example showing how you use id/descriptions to make it more clear?

@ghost
Copy link
Author

ghost commented Nov 12, 2020

I don't really get what's blocking you from using your own ids in id values, can you share a minimum example showing how you use id/descriptions to make it more clear?

ID uniqueness is the primary reason, according to the API.md docs, https://github.com/VROOM-Project/vroom/blob/master/docs/API.md

id: an integer used as unique identifier

If as you say IDs can be repeating, then it's certainly not an issue! If there is such a guarantee for non-uniqueness then the docs should be updated too.

Secondary reasons was future plans to store our own (stringified) JSON's for our own internal extra info, e.g.:
description: "{internal_id: 1, weight: 99, depot: 5}"

@jcoupey
Copy link
Collaborator

jcoupey commented Nov 12, 2020

The current behaviour is to not enforce any kind of uniqueness on job/pickup/delivery task id values, so you can practically use the same id across different tasks. I don't see how this is a good solution, because then you won't be able to differentiate tasks in solutions. This is the reason why uniqueness is mentioned in the docs.

I think the right thing to do at some point is to enforce uniqueness for (id, type) pairs so one could differentiate tasks with same id based on type (makes sense for shipment objects where the same database id is used for both pickup and delivery). In that case we should provide type in output for unassigned tasks.

Secondary reasons was future plans to store our own (stringified) JSON

See similar discussion back in #243, it looks like a workaround where a cleaner client-side approach could be used.

@ghost
Copy link
Author

ghost commented Nov 12, 2020

See similar discussion back in #243, it looks like a workaround where a cleaner client-side approach could be used.

In our particular case input files are generated by front-end task and put in a queue. The backend once a while runs that queue, gets results, parses and processes them. It would be really convenient to have a reliable way to pass information some way (e.g. through 'description'). Otherwise we have to parse input files in the backend task just to get that information.

We can live with that, but I don't understand the logic for not doing it: Why add back 'description' to 'steps', but not to 'vehicles' (start/end) or 'unassigned'? If the reason is more convenient plotting on the map for 'steps', then why not for 'vehicles' or 'unassigned' -- we want more convenient plotting there, too.

@jcoupey
Copy link
Collaborator

jcoupey commented Nov 13, 2020

Yeah, to some extent there is no foolproof logic behind this, because it's the result of successive subjective choices and a trade-off between convenience and not bloating the API.

At this point we could come to a more consistent output by adding the description value to the unassigned objects. In the light of previous comments, we should also add the task type (job, pickup or delivery), and maybe mark the location key as deprecated. I'd be happy to merge a PR for this.

I see the question of vehicle start and end differently as the description key apply to their parent object (job, pickup, delivery or vehicle), not their location(s). If you want to pass info on vehicle start and end, you could probably simply embed it in some way in a single string and get it back afterwards.

@ghost
Copy link
Author

ghost commented Nov 13, 2020

Yeah, to some extent there is no foolproof logic behind this, because it's the result of successive subjective choices and a trade-off between convenience and not bloating the API.

I agree. Thanks for considering this feature. I'll try to work on it.

@jcoupey jcoupey changed the title add 'description' to unassigned & start/end steps add 'description' to unassigned Oct 4, 2021
@jcoupey jcoupey added this to the v1.12.0 milestone Jan 6, 2022
@jcoupey jcoupey added the API label Jan 6, 2022
@jcoupey jcoupey closed this as completed in 7c40d23 Jan 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant