NYUCCL / psiTurk

An open platform for science on Amazon Mechanical Turk.
https://psiturk.org
MIT License
277 stars 140 forks source link

configuration defaults and backwards compatibility #515

Closed gureckis closed 2 years ago

gureckis commented 3 years ago

I have run into two examples now where I must not be understanding the intention behind the backwards compatibility feature in psiturk_config.py.

Using the current master, my code would not run on a fresh install. The reasons what that the server was crashing when looking up the value of errorlog. My local config.txt defined logfile=server.log and had no entry for errorlog. In debugging this I found that errorlog was being automatically set to an empty string. Looking at local_config_defaults.txt I saw this code block:

errorlog = 
# For backwards compatibility, `logfile` is synonymous with `errorlog`. If
# both are set, `errorlog` will be preferred over `logfile`.
logfile = server.log

Now even by the logic of the comment there errorlog should take prescendence and, is therefore an empty string, causing my code to crash.

In recent commit I reversed this, setting errorlog=server.log and commenting out the logfile default. I felt this made sense because the backwards compatibility code says "prefer" errorlog over logfile.

Several tests fail now on line because it is asking for 'logfile' but there is not default value provided.

My understanding is that when a request is made for the 'logfile' option it should revert to 'errorlog' because they are synonymous and so only one needs to be set. Is this not right?

My second example of problems has to do with the recent PR that I made with config options where again the backwards compatibility options are not seeming to be "synonymous" (tried to alias table_name and assignment_table_name).

I happy to dive into the backwards compatibility code and see if there is an error but I feel like first making sure I understand how this is supposed to be working before seeing if it is really broken!

deargle commented 3 years ago

The logic isn't too complex after a recent change, see https://github.com/NYUCCL/psiTurk/blob/d081cadef3d2fe22b4bf7d64a11faf89272295eb/psiturk/psiturk_config.py#L100-L107

The reasons what that the server was crashing when looking up the value of errorlog

errorlog should be okay being blank because blank will return false for if preferred in the codeblock above. But it looks like I forgot to change experiment_server.py to look for logfile instead of errorlog. I'm going to revert your latest commit because I recently made a commit that it reverted lol. Your code should work after that. It sounds like we need a unit test that tries to launch the experiment_server.py, to see if any errors happen!

My second example of problems has to do with the recent PR that I made with config options where again the backwards compatibility options are not seeming to be "synonymous" (tried to alias table_name and assignment_table_name).

I changed the logic from when you were trying to do that to be more simple -- is it still not working?

deargle commented 3 years ago

PS, one benefit of using pull requests for commits is that it will trigger unit tests to run, if you don't want to run them locally.

deargle commented 2 years ago

There was a bug in the backwards-compatibility lookup that #528 closed. I'm guessing that the bug was leading to the unexpected behavior, lmk if otherwise.