Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

small doc improvements from user testing #97

Merged
merged 16 commits into from Apr 21, 2021
Merged

small doc improvements from user testing #97

merged 16 commits into from Apr 21, 2021

Conversation

nhoening
Copy link
Contributor

@nhoening nhoening commented Apr 9, 2021

Some comments which came from a user conversation today.

I also wonder about one more thing I cannot judge by myself: The horizon parameter is not considered optional any more for some API versions < v2 (under some circumstances), but the docstring still list it under **Optional fields** (v1/routes.py::post_meter_data). Even the validator is called optional_horizon_accepted but I'm not sure if that paints the real picture by now or could be more clear.

@nhoening nhoening requested a review from Flix6x Apr 9, 2021
@Flix6x
Copy link
Contributor

@Flix6x Flix6x commented Apr 11, 2021

Right now, for API versions <2, the horizon field is only optional for servers in play mode (set to 0 hours by default), but perhaps we can use the same default for all modes of operation, by removing line 415 in validators.py.

Flix6x
Flix6x previously approved these changes Apr 11, 2021
documentation/dev/data.rst Show resolved Hide resolved
@nhoening
Copy link
Contributor Author

@nhoening nhoening commented Apr 12, 2021

So your suggestion is to remove line 415 in validators.py, which makes the field not optional anymore, and then also adjust the documentation / docstrings I mentioned?

@Flix6x
Copy link
Contributor

@Flix6x Flix6x commented Apr 12, 2021

No, removing line 415 actually makes the horizon field completely optional again (a missing horizon will be turned into a 0 horizon). As far as I can tell at the moment, only lines 380 and 381 in the corresponding docstring would need to be adjusted. It's worth an API changelog entry, too, though. It kind of rolls back the 3rd breaking change under v2.0 | 2021-04-02. I write "kind of" because the default inference (for non-play servers) used to be to set the belief time equal to the server time (leading to different horizons for each belief), whereas now (when deleting line 415) the default inference is to set the belief horizon to zero (leading to the same horizon for each belief).

@nhoening nhoening requested a review from Flix6x Apr 13, 2021
@nhoening nhoening changed the title small impovements from user testing small doc improvements from user testing Apr 13, 2021
@Flix6x Flix6x dismissed their stale review Apr 13, 2021

Need to take another look, please hold

Flix6x
Flix6x approved these changes Apr 14, 2021
Copy link
Contributor

@Flix6x Flix6x left a comment

I hope it makes more sense now, and I'm glad to see the horizon is now optional again for all API versions.

For future reference, we ran into this because I failed to avoid that 3rd breaking change from #41.

@@ -3,6 +3,10 @@
API change log
===============

v2.0 | 2021-04-XX
"""""""""""""""""
- [**Breaking change**] Automatic inference of horizons for Post requests re-instantiated, now inferring a belief horizon of zero (leading to the same horizon for each belief).
Copy link
Contributor

@Flix6x Flix6x Apr 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait, no. Describing this as a breaking change for v2.0 would be wrong for two reasons:

  • It should not impact v2.0, but versions <2.0 only. Version 2 already inferred a prior (for non-play servers) or a horizon (for play servers) instead.
  • It is not a breaking change, but partially reverting a breaking change by reinstantiating the inference, but changing the inference policy.
    I pushed a commit that should fix this issue.

My remaining concern is about structuring the API changelog itself. The entry is listed under v2, but the change only impacts lower versions. I don't have a good answer on how to restructure, but I can explain how I handled this before.

The entries are not listed chronologically, but grouped by the most recent API version they impact, with some earlier entries also impacting multiple versions (which is noted with a comment).

I suggest we move the 3rd breaking change of v2.0-2 to a new entry v1.3-8 (with the same date as v2.0-2 and a comment *Affects all versions since v1.0*.), and to move the current entry v2.0-3 to a new entry v1.3-9, also with a comment *Affects all versions since v1.0*.

Copy link
Contributor Author

@nhoening nhoening Apr 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"I pushed a commit that should fix this issue." Which one do you mean?

Copy link
Contributor

@Flix6x Flix6x Apr 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apparently I forgot to actually push my commit. I did just now, after resolving merge conflicts in the changelog.

@nhoening nhoening requested a review from Flix6x Apr 15, 2021
Flix6x
Flix6x approved these changes Apr 15, 2021
Copy link
Contributor

@Flix6x Flix6x left a comment

I approve, but now you should probably review my changes in c5923ae.

@nhoening
Copy link
Contributor Author

@nhoening nhoening commented Apr 15, 2021

So maybe now is the time to document what we mean with this sentence in the configuration documentation if FLEXMEASURES_MODE: "This is used to turn on certain extra behaviours." There are two modes our code recognizes, demo and play.

  • Demo: ability to display login credentials, ability to keep data from one year, but keep FlexMeasures functioning with all other years as well. Both of these are controlled with other config settings. Anything else?
  • Play: Can you fill in what happens in play mode?

@optional_prior_accepted()
@optional_horizon_accepted(
infer_missing=True
if current_app.config.get("FLEXMEASURES_MODE", "") == "play"
Copy link
Contributor Author

@nhoening nhoening Apr 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Flix6x The use of current_app "outside" the function like this is not possible, as there is no application context at that point. I learned that from all our tests falling, as importing this file breaks.

We have to find a different way. If, for play mode, infer_missing is always True for horizons ad False for prior, maybe it could go into the respective validators.

Copy link
Contributor

@Flix6x Flix6x Apr 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for finding out. I'll move the logic back into the validators, and devise of some other way of letting v1.x and v2 endpoints choose to use this logic. Alas, infer_missing is not always True for horizons in play mode. Specifically, it is False for the postPrognosis endpoint v1.x, even in play mode. I could make the boolean optional, and set the default to None implying inference, so endpoints can be explicit or let the validator choose a default depending on server mode.

For the horizon validator:

  • v1.x postPrognosis: infer_missing=False
  • v1.x postOther: infer_missing=True
  • v2.x postPrognosis: infer_missing=False
  • v2.x postOther: infer_missing=None -> True if in play, False otherwise

For the prior validator:

  • v1.x doesn't use this validator
  • v2.x postPrognosis and postOther: infer_missing=None -> False if in play, True otherwise

So to be clear, the differences between v1.x and v2 are that:

  • The only time a user needs to be bothered with communicating the timing of their beliefs is for the postPrognosis endpoint: for v1.x this is in all modes, whereas for v2, this is in play mode only.
  • The inference policy for v1.x is beliefs formed right after the fact, whereas for v2, this is beliefs formed when communicated to FM.

@Flix6x
Copy link
Contributor

@Flix6x Flix6x commented Apr 16, 2021

So maybe now is the time to document what we mean with this sentence in the configuration documentation if FLEXMEASURES_MODE: "This is used to turn on certain extra behaviours." There are two modes our code recognizes, demo and play.

  • Demo: ability to display login credentials, ability to keep data from one year, but keep FlexMeasures functioning with all other years as well. Both of these are controlled with other config settings. Anything else?

Yes, a lot. I did this exercise for play mode just now, and compiled the list below. I suggest you make a separate issue for this, because it looked like there had been even more customisation for the demo mode.

  • Play: Can you fill in what happens in play mode?

Play

Use this mode to turn on several FlexMeasures features offering better support for simulations.

Big features

  • [Data] Allows overwriting existing data when saving data to the database.

  • [API] The inferred recording time of incoming data is immediately after the event took place rather than the actual time at which the server received the data.

  • [API] Posting price or weather data does not trigger forecasting jobs.

  • [API] Registers the restoreData endpoint enabling database resets through the API.

  • [API] Automatically creates a new weather sensor when posting weather data for a new location, instead of returning the nearest available weather sensor to post data to.

Small features

  • [API] Posted UDI events are not enforced to be consecutive.

  • [API] Names in GetConnectionResponse are the connections' unique database names rather than their display names (this feature is planned to be deprecated).

  • [UI] The dashboard plot showing the latest power value is not enforced to lie in the past (in case of simulating future values).

  • [Documentation] The restoreData endpoint is left out of the changelog. [I don't think this works anymore in the new public non-server-bound documentation].

@nhoening
Copy link
Contributor Author

@nhoening nhoening commented Apr 16, 2021

Thanks! I can cover demo mode.
I believe it would make a nice addition to this random-docs-improvements PR.

Flix6x
Copy link
Contributor

@Flix6x Flix6x commented on 945fbbb Apr 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

>

Flix6x
Copy link
Contributor

@Flix6x Flix6x commented on 945fbbb Apr 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks really good.

Flix6x
Copy link
Contributor

@Flix6x Flix6x commented on 945fbbb Apr 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

>

Flix6x
Copy link
Contributor

@Flix6x Flix6x commented on 945fbbb Apr 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks really good.

@nhoening
Copy link
Contributor Author

@nhoening nhoening commented Apr 19, 2021

I believe how that explicit decorator parameter could become clearer is by calling it "infer_X_override_modes" and it receives a list. By default the list is empty so we don't need it always. And we could expand it by other modes.

@Flix6x
Copy link
Contributor

@Flix6x Flix6x commented Apr 20, 2021

We discussed adjusting the decorator parameter infer_missing_play to something that overrides the infer_missing parameter, for play or some list of server modes, by instructing the validator to do the opposite of whatever the infer_missing parameter tells it to do. After some more consideration I would like to vote against such renaming, with the following arguments:

  • Letting the interpretation of one parameter depend on another makes setting up the decorator more prone to error.
  • I plan to move the optional_horizon_accepted and optional_prior_accepted to a polyfield marshmallow decorator, including replacement of the mode dependent inference policy with a dedicated config variable.

@nhoening
Copy link
Contributor Author

@nhoening nhoening commented Apr 20, 2021

As I said earlier, I won't force my opinion through here. Let's get this thing out the door.

@Flix6x Flix6x added this to the 0.4.0 milestone Apr 20, 2021
@nhoening nhoening merged commit 8ed869d into main Apr 21, 2021
2 checks passed
@nhoening nhoening deleted the small-doc-improvements branch Apr 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants