fohrloop / dash-uploader

The alternative upload component for python Dash applications.
MIT License
144 stars 30 forks source link

Support the "prevent_initial_call" option for "du.callback". #37

Closed cainmagi closed 3 years ago

cainmagi commented 3 years ago

Support the prevent_initial_call option when dash>=1.12.0.

The option prevent_initial_call is designed for improving the efficiency of the callbacks since dash==1.12.0. If this option is enabled, the callback would not be fired during the initialization. An official example could be referred here.


To author:

  1. Currently, I have only modified the codes. If you think this pull request is acceptable, please let me know. I could also update the documentation before you merge it. Thank you!
  2. In this project, the callback should be always fired after the file uploaded, so I think that the initial fire is not required. Although prevent_initial_call is set False by default in dash, it is better to make prevent_initial_call=True in our case. If you agree with this suggestion, I could update the default value.
fohrloop commented 3 years ago

Hi again @cainmagi and thank you for your PR! I would merge this in, but I am asking myself the question "what problem does this solve?". Could you demonstrate what is the problem before this PR?

Did you notice that there is somewhat quick-n-dirty piece of code that should(?) prevent the initial callbacks from firing? Namely, in the dash_uploader\callbacks.py, the following lines:

    def wrapper(iscompleted, filenames, upload_id):
        if not iscompleted: # <-- this
            return

        out = []
        if filenames is not None: # <-- and this
            if upload_id:
                root_folder = Path(settings.UPLOAD_FOLDER_ROOT) / upload_id
            else:

I think that in the beginning, the isCompleted input will be False and the filenames will be None. Do you think there is some problem with this approach?

cainmagi commented 3 years ago

Thank you for replying to me. Actually, I think this PR is not for fixing a "problem". I create it just because the newer dash supports this feature, and I want to make this project catch up with the newest dash features.

I know that your codes have already implemented this feature, and this is a typical way for skipping the initial fire. This implementation is a little bit different from prevent_initial_call. In comparison, if prevent_initial_call=True, the frontend (browser) would not send the request to the server-side. Because the initial fire does not exist, we do not need to write codes for skipping it.

However, I think the "quick-n-dirty" pieces are still necessary here unless we always make prevent_initial_call=True. Because users may not set prevent_initial_call=True, these two pieces perform a double check and make the codes more robust.

fohrloop commented 3 years ago

In comparison, if prevent_initial_call=True, the frontend (browser) would not send the request to the server-side. Because the initial fire does not exist, we do not need to write codes for skipping it.

If this is the case, then I think this definitely is fixing something! For every user, each time the app gets loaded, there would be unnecessary HTTP request to the server, right?

Now I am thinking, what is the additional gain if this component would have an initial callback call with the default arguments? Is there any use case for that? I guess if dash app developers want to know when their app is initialized on users' browser, they could add an another component for that kind of "spying". I am thinking if this should be the hard coded default for the component. Also, this would release some mental overhead from new users as there would be less parameters for du.callback.

cainmagi commented 3 years ago

Yes. Without prevent_initial_call, there would be always a POST /_dash-update-component request during the initialization. I agree that making prevent_initial_call=True hard-coded is not a good option.

  1. prevent_initial_call is only provided after dash 1.12.0. However, dash-uploder support dash>=1.1.0. For the compatibility issue, it is necessary to fall back to python skipping if the dash version is not new enough.
  2. Since app.callback provides prevent_initial_call option, I think du.callback should provide similar interface.
  3. For the case you mentioned (require a "spying" component), if we expose the prevent_initial_call option to users, users could make it False to meet their requirement.

I have checked the source codes of dash, and I find that the default value of prevent_initial_call in app.calback is None (See here). This is because dash provides another option prevent_initial_callbacks in dash.Dash (See here). I think it is better to change the default value to None for making the global value working. If you agree that, I could start to do it.

fohrloop commented 3 years ago

I agree that making prevent_initial_call=True hard-coded is not a good option.

I actually tried to say that it would be a good idea to just hard-code the prevent_initial_call=True, when dash version is higher or equal to 1.12.0.

prevent_initial_call is only provided after dash 1.12.0. However, dash-uploder support dash>=1.1.0. For the compatibility issue, it is necessary to fall back to python skipping if the dash version is not new enough.

Yeah that's how it must be done to support dash versions less than 1.12.0. I completely agree with you.

Since app.callback provides prevent_initial_call option, I think du.callback should provide similar interface.

I understand your point. It seems like a nice idea to have similar looking API for du.callback as for the Dash.callback.

However, as a user of an upload component, I would rather have as simple interface as possible. I am kind of thinking the typical use case. Someone wants to upload files to dash app --> He/she finds dash-uploader and uses du.Upload --> He/she wants to have a callback and uses du.callback. I imagine that 99.9% of users are not interested to know when the du.Upload component was mounted and surely wont want their servers to be bombed with HTTP requests that have no meaning to them. The typical user would only ever even see that this is happening, when either reading the docs very carefully, or when reading some logs or using the Chrome Developer tools, again, very carefully. Only some advanced users with very specific use case would want to enable the "component mounted" callback - I assume.

What would user think?

I think there are three different options with their pros and cons:

1. Remove support for prevent_initial_call and just make it always be True.

2. Give option for prevent_initial_call in du.callback and make the default True.

3. Give option for prevent_initial_call in du.callback and make the default False.

To me, from all the choices, best would be to just prevent any initial callbacks for the reasons given above. If you think having possibility to track mounting of the component is really must have, then I would suggest the option 2.

cainmagi commented 3 years ago

Thank you. I understand your preference now. I would change to option 1 in the next commit.

cainmagi commented 3 years ago

I just noticed one phenomenon. To make prevent_initial_call work, it is required to add one line for initializing the component in upload.py:

arguments = dict(
    id=id,
    isCompleted=False,  # This line is added for making `prevent_initial_call` work.
    maxFiles=max_files,
    ...
)

To understand this change, we need to refer to the official document:

It is important to note that prevent_initial_call will not prevent a callback from firing in the case where the callback's input is inserted into the layout as the result of another callback after the app initially loads unless the output is inserted alongside that input! In other words, if the output of the callback is already present in the app layout before its input is inserted into the layout, prevent_initial_call will not prevent its execution when the input is first inserted into the layout.

In our case, prevent_initial_call would not work, if the callback input isCompleted has not been configured during the initialization.

I have added this line and checked the performance. It works well now, which means:

  1. The initialization would not fire the callback.
  2. Uploading a file would fire the callback.
  3. If I change the hard-coded argument to prevent_initial_call=False , the callback would be always fired.

But I am not sure whether this change would cause any side effects (I have not written custom components before). Please help me confirm that. Thank you!

fohrloop commented 3 years ago

Good catch! I tested it myself and it works just as you described. Thank you for your work, it is now part of the dev branch! 🎉

cainmagi commented 3 years ago

Thank you!