uma-pi1 / kge

LibKGE - A knowledge graph embedding library for reproducible research
MIT License
773 stars 125 forks source link

Minimized configs for reciprocal_relations_model are missing base_model import #89

Closed samuelbroscheit closed 4 years ago

samuelbroscheit commented 4 years ago

The minimized configs for our best_models that use reciprocal_relations_model are missing the base_model import.

This leads to

KeyError: "Key 'distmult.entity_embedder.dropout' cannot be set because key 'distmult' does not exist and no new keys are allowed to be created at root level."

for our best model

http://web.informatik.uni-mannheim.de/pi1/iclr2020-models/fb15k-237-distmult.yaml

Possible fixes:

  1. minimal configs should retain the import statement per default
  2. reciprocal_relations_mode.base_model triggers base model import
AdrianKs commented 4 years ago

we should also remove specific GPU indentifiers from the config files.

As for example change: job.device: cuda:3 to job.device: cuda

otherwise people with only one gpu can't run it

samuelbroscheit commented 4 years ago

Yes saw that too, good suggestion.

rgemulla commented 4 years ago

Missing import statement in exported config fixed in 47e575f.

@rufex2001: I am leaving this open until the relevant minimal configs are regenerated.

rgemulla commented 4 years ago

we should also remove specific GPU indentifiers from the config files.

Yes, although I suggest to remove the device completely for any configs on the website (instead of renaming it).

rufex2001 commented 4 years ago

Minimal configs updated in kge-iclr20 repo.

rgemulla commented 4 years ago

But not yet in this one.

rufex2001 commented 4 years ago

Servers have been updated.

samuelbroscheit commented 4 years ago

The job.device is still in the configs.

rufex2001 commented 4 years ago

How do we implement this? Remove from all types of config dumps? Just minimal? I suggest to keep it in raw and full because their names suggest that, but drop it from minimal. Rainer's suggestion says "from the website", but I'd like to keep this automated, rather than having to remove them manually if the dumps will still include them.

On Tue, 14 Apr 2020 at 12:43, samuelbroscheit notifications@github.com wrote:

The job.device is still in the configs.

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub https://github.com/uma-pi1/kge/issues/89#issuecomment-613364649, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABEWXZE2DKGSET5ZVYYARPTRMQ45JANCNFSM4MC26EEA .

samuelbroscheit commented 4 years ago

kge dump config could take option --keep (f.ex. import) and --drop (f.ex. job.device) which we can use to create the website configs. This keeps it automatic.

rgemulla commented 4 years ago

You mean kge dump config. The parameters you suggest are already there and called --exclude and --include.

rufex2001 commented 4 years ago

That feature is already there, so I can already use it for automation. I thought we wanted to make it more permanent by removing this by default.

On Tue, 14 Apr 2020 at 15:04, samuelbroscheit notifications@github.com wrote:

kge dump trace could take option --keep (f.ex. import) and --drop (f.ex. job.device) which we can use to create the website configs. This keeps it automatic.

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub https://github.com/uma-pi1/kge/issues/89#issuecomment-613429985, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABEWXZACRVNZN5ZSIK2LZYDRMRNN5ANCNFSM4MC26EEA .

samuelbroscheit commented 4 years ago

Yes just realized this, didn't know, cool.

rufex2001 commented 4 years ago

How about now?

rgemulla commented 4 years ago

Looks good. Please close once done.