paris-saclay-cds / ramp-board

RAMP packages: database, backend, frontend, utilities
https://paris-saclay-cds.github.io/ramp-docs/
BSD 3-Clause "New" or "Revised" License
13 stars 17 forks source link

BUG deploy-event with `--force` overwrites problem and event #428

Open maikia opened 4 years ago

maikia commented 4 years ago

when deploying a new event with a --force flag: ramp setup deploy-event --event-config events/iris_test/config.yml --no-cloning --force

the event is being closed properly to deploy it again (here iris_test), however the other events which are using the same starting kit (here all iris events) are also closed

Description

Steps/Code to Reproduce

Expected Results

Actual Results

Versions

lucyleeow commented 4 years ago

Hmm it says here that the --force flag:

Whether or not to overwrite the problem if it exists [default: False]

So maybe we actually don't need to add that to re-deploy?

maikia commented 4 years ago

But if the things changed and if we need to overwrite it should not kill all the events, just the one we want to change

lucyleeow commented 4 years ago

Ah I see, I misunderstood!

maikia commented 4 years ago

Wait. Maybe its me who misunderstands. If the --force is responsible for changing the whole problem and not the event maybe thats not the right place to have it? In my case only the config.yml file for the event changes and I need to kill the previous running event and deploy it again, but the problem itself is not changing. Maybe thats the wrong flag to use in this case?

lucyleeow commented 4 years ago

Yes, if by 'problem' the doc means the 'iris' challenge/problem, then we do not need force to re-deploy just one event.

maikia commented 4 years ago

If event is deployed, redeploying will not work without the force flag

lucyleeow commented 4 years ago

I think the problem is here: https://github.com/paris-saclay-cds/ramp-board/blob/0fe2450f5cbc4f4c2204eb628631f4cf21ad775a/ramp-utils/ramp_utils/deploy.py#L79-L83

From the fun deploy_ramp_event:

force : bool, default is False
       Whether or not to potentially overwrite the repositories, problem and
       event in the database.

I don't know details but it seems that force can potentially overwrite the problem AND event, but we should keep the overwriting behaviour separate?

cc @agramfort @rth

lucyleeow commented 4 years ago

I think the current/lazy 'solution' is to delete an event and make a new one, instead of overwritting an existing event

agramfort commented 4 years ago

just to be sure I follow. What do you expect from a redeploy? all events use the same files (starting kit, problem etc). So when you redeploy it must affect all events right?

maikia commented 4 years ago

They use different config.yml

It seems that if I redeploy with --force all the running events for the given problem are killed, together with their submissions etc, and only the given event is redeployed completely fresh. The others need to be then deployed again manually.

lucyleeow commented 4 years ago

re-deploy an event NOT a problem. e.g., if we have an existing event, but need to change something in the event and re-deploy. You can update simple things of an event (e.g., name, end time) but if you change more complex thing (e.g., we are altering the aws settings in the config.yml file of an event) I think you need to re-deploy

But if it isn't a common occurrence, we can stick with deleting event and re-deploying it.

maikia commented 4 years ago

Ok. Thats a solution but then we need to make it very clear. For now if you try to deploy running event it will fail suggesting using --force. There is no mention about other events being killed. There is no feedback on it. I just happened to notice that the others disappeared

lucyleeow commented 4 years ago

Yes there should definitely be a note, I'm just suggesting the priority for which bugs/enhancement to address first

agramfort commented 4 years ago

when you redeploy what is the part you typically want to change?

lucyleeow commented 4 years ago

event config.yml file

agramfort commented 4 years ago

ok but more specifically the worker and the dispatcher?

maikia commented 4 years ago

the worker. I want to completely restart the worker with the new config.yml. That's why the solution suggested by @lucyleeow would work for my case. "the current/lazy 'solution' is to delete an event and make a new one, instead of overwritting an existing event"

but this does not solve the problem of --force being confusing and risk of killing running events without users intention

agramfort commented 4 years ago

ok then it seems we have a quick solution and a more ambitious thing to fix !

lucyleeow commented 4 years ago

Let's update the documentation for now. We can work on this later but I think the docker issue (#290) is higher priority.

agramfort commented 4 years ago

+1