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

Introduce Solution Model #309

Closed
wants to merge 1 commit into from
Closed

Conversation

braktar
Copy link
Contributor

@braktar braktar commented Nov 15, 2021

No description provided.

@braktar braktar mentioned this pull request Nov 15, 2021
5 tasks
@braktar braktar marked this pull request as ready for review November 17, 2021 08:02
api/v01/vrp.rb Show resolved Hide resolved
@@ -170,48 +171,42 @@ class Vrp < APIBase
id = params[:id]
job = Resque::Plugins::Status::Hash.get(id)
stored_result = APIBase.dump_vrp_dir.read([id, params[:api_key], 'solution'].join('_'))
solution = stored_result && Oj.load(stored_result) rescue Marshal.load(stored_result) # rubocop:disable Security/MarshalLoad, Style/RescueModifier
job_object = stored_result && Oj.load(stored_result) rescue Marshal.load(stored_result) # rubocop:disable Security/MarshalLoad, Style/RescueModifier
Copy link
Contributor

Choose a reason for hiding this comment

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

why not result_object or solution_object or just result / solution?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, the object stored in redis is neither a single solution nor a single result.
It holds all the properties of the job and which contains a key called :result which is collection of solutions.
By changing the naming, I want to separate the internal objects from the objects manipulated on the api side to avoid confusion

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes but the part that is dumped (and loaded here) is a result, is it not?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We directly store the content of the job returned through Redis

Copy link
Contributor

Choose a reason for hiding this comment

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

I think I understand the differentiation you want to make but I don't think job_object is the way to go, because we have a class Job and real objects of this class which is confusing.

I know that it is not directly a single result nor a solution but at the end of the day we do get/set this "object" via OptimizerWrapper::Result class that's why I suggest calling it result_object:

result_object ||= OptimizerWrapper::Result.get(id)

With the suffix _object we give a heads-up that it is not exactly solution[:result]. Would that work for you?

models/concerns/as_json.rb Show resolved Hide resolved
models/concerns/validate_data.rb Show resolved Hide resolved
Copy link
Contributor

@senhalil senhalil left a comment

Choose a reason for hiding this comment

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

Needs a rebase 😢

@braktar braktar closed this Jan 11, 2022
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.

2 participants