Repository navigation
Handling filters with None #371
Description
Passing a None object as a filter argument on listing calls seems to ignore the filter rather than returning no results.
E.g. list_evaluations(setup=setuplist) where setuplist happens to be None will happily download ALL OpenML evaluations (when placed in a paging loop).
Indeed, the code says:
def list_evaluations(function, offset=None, size=None, id=None, task=None,
setup=None, flow=None, uploader=None, tag=None):
if setup is not None:
api_call += "/setup/%s" % ','.join([str(int(i)) for i in setup])
It would be safer to actually check whether the argument was passed? This can be done with kwargs: https://stackoverflow.com/questions/14749328/python-how-to-check-whether-optional-function-parameter-is-set
What do you think? This actually happened to me by mistake (I assumed all studies have a list of setups - not) and I only noticed after downloading 1.3 million evaluations.
Normally you wouldn't worry too much about stupid user mistakes, but in this case it stresses the server. Maybe adding it just for the listing calls isn't the worst idea.
Activity
chouhanaryan commentedon Oct 18, 2020
Hi, is this still open? Could you point me towards the relevant code? I'd like to contribute, thanks.
PGijsbers commentedon Oct 19, 2020
Hi @chouhanaryan, thanks for taking an interest! Let me clarify some things with other OpenML members to see how we would like to see this issue addressed.
@joaquinvanschoren @mfeurer
I understand that user errors may specify
list_evaluationswithout filters unintentionally (i.e. as in the original post your variable isNonewhen you don't expect it to). I don't think that @joaquinvanschoren's solution would work. Whenlist_evaluations(setups=setuplist)is used withsetuplist=None, we still seesetupsas being explicitly set (toNone). The second disadvantage of usingkwargsis, as far as I am aware, that intellisense type features as well as type checking are not compatible with it. We could use key-word only arguments to mitigate this, but it does not solve the underlying issue: ifsetuplistsisNonethen all evaluations are still downloaded.I think there are two straight-forward ways to avoid accidentally downloading all evaluations:
size(e.g.10_000, I don't know what's reasonable), orNonewhenlist_evaluationsis called. In case all arguments areNone, either set a defaultsize, or raise an exception (users wanting to download all results can still do so by settingsizeto a high number like 100M).I think I would be in favor of option 1, it's straight forward and easy to explain. It will also throttle other (perhaps unexpectedly large) queries.
mfeurer commentedon Oct 19, 2020
Option 1 appears reasonable to me.
PGijsbers commentedon Oct 20, 2020
@chouhanaryan If you still feel up for the task, then you can get started with changing the default
sizeforlist_evaluations. This includes:sizetoNoneremoves a limit to the number of results returned).sizeshould be set back toNonefor those usages. This includes usages in unit tests (i.e. change them where it would fail otherwise, or where it looks reasonable that>10_000results are expected).If you have any questions, feel free to ask!
@mfeurer I think it makes sense to ask the same for
list_evaluations_setups?mfeurer commentedon Oct 20, 2020
The more I think about this, the less I actually like the fact that we change the behavior of a few listing call while keeping the other listing calls return everything. Therefore, I would prefer to for now restrict this to only
list_evaluationsuntil we (hopefully) find a better solution.PGijsbers commentedon Oct 21, 2020
Works for me. Though I don't think it is unreasonable to add an (equal) max size to each listing call. Perhaps make it a bit higher (e.g.
100_000) so that only the most demanding calls will need to overwrite it.mfeurer commentedon Oct 21, 2020
Is there a chance to find out how many elements are online via an API call? If yes, we could emit a warning in case a user requests too few elements.
chouhanaryan commentedon Oct 21, 2020
Hi! Sorry for the delay. I've been a bit busy. I'll work on this over the weekend.
Unfortunately I don't use PyCharm so I'm not sure if I can complete the fourth task you mentioned @PGijsbers , but I'll try to find an alternative.
Thanks!
PGijsbers commentedon Oct 21, 2020
@mfeurer
I think the warning can make sense specifically only when we see that
NUMresults are returned, whereNUMis whatever default maximum we decide on. I.e., assuming10_000is our default max:list_datasets(size=10)does not omit a warning, despite more results being availablelist_evaluations(size=1_000_000)does not omit a warning, despite more results being availablelist_evaluations()does omit a warning, because10_000amount of results are returned andsizewas left at10_000.It would be trivial to check this scenario (
len(results)==NUM). The only issue is that we can not detect ifsizewas set toNUMexplicitly or by default. I think it's fine to ignore this (and omit a warning even iflist_evaluations(size=10_000)is called).I am not aware of a call that allows you to see how large the results could be. But I'm not convinced it's worth the trouble to add it. Just state clearly that only
10_000results (or whatever number we agree on) are returned by default.@chouhanaryan Great to hear you're still interested! Most editors allow you to search all files (often
ctrl+shift+F), which would also be suitable 👍 Let us know if you run into any issues.mfeurer commentedon Oct 21, 2020
Then I guess we'll go with the easy solution proposed.
PGijsbers commentedon Oct 21, 2020
To clarify, with or without warning? I don't feel strongly for adding a warning as long as the documentation is clear. But it's not an issue to me to have it added either.
We could also consider having a release that does not change behavior but issues a
FutureWarningindicating the default will change. Personally I don't feel the change is invasive enough to skip a release cycle, but we have no numbers to back this up.mfeurer commentedon Oct 21, 2020
Sorry, without a warning. I'd also go without a
FutureWarning.PGijsbers commentedon Oct 21, 2020
@chouhanaryan For clarification, these are the final proposed changes to resolve this issue. The discussion after can be ignored as it did not lead to changes in the requirements.