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

Frontend state & data flow rework #37

Open
wants to merge 20 commits into
base: main
Choose a base branch
from
Open

Frontend state & data flow rework #37

wants to merge 20 commits into from

Conversation

nking-1
Copy link
Contributor

@nking-1 nking-1 commented Nov 2, 2022

Reworks the way that Adatest's front end handles data caching and refreshes stale data.

Description of work

  • Data fetched from backend is now stored in a Redux store, whereas previously it was cached in a custom class. Redux gives us nice debugging tools and an idiomatic way of working with React's rendering model.
  • The frontend is now responsible for fetching data when it's stale, instead of receiving new data pushed proactively by the backend. This allows the client-side code to have much more control over when components re-render. There have previously been issues with the frontend re-rendering with the results of long-running operations (for example, computing model output), overwriting whatever the user was doing in the meantime.
  • Every websocket message is now assigned a sequence number so we can track its flow. The backend is expected to respond with an 'ok' along with the corresponding sequence number if the operation worked. This will allow the frontend to respond to timeouts or other backend errors gracefully.
  • TypeScript support added everywhere, mainly used for specifying the types stored in each component's props and state. This will make it easier for everyone to see exactly what data a component expects. I didn't enable strict mode, so hopefully it won't be too annoying for anyone who wants to write dynamically typed code. However, I did enable strict null checks, because it helped me fix several bugs that were lurking in the code base.

Implementation details

One slightly strange thing I had to do was figure out how to make our class-based components work with React's hooks, which can only work with function components. This was necessary because hooks have become the new standard for React libraries like Redux, and the new Redux Toolkit removes a lot of boilerplate previously required for Redux.

The general idea is to wrap a class component with a function component which is able to use hooks and pass on anything needed as props to the class component. In our case, the function components fetch data from the Redux store and pass it on as props for the class components, which allowed me to reuse almost all the logic inside the original class components.

The main files for Redux are store.ts and TestTreeSlice.ts.

Known issues

Things are mostly working, but there are still some bugs to fix. Work still in progress:

  • ContentEditable is still buggy. I am still figuring out how to make it work gracefully with the new data flow.
  • UI lag caused by aggressively refreshing data
  • Code cleanup - this was a large change and I'd like to make it less disruptive if possible. There is also some code I've commented out that I should delete.

@nking-1 nking-1 marked this pull request as ready for review November 22, 2022 23:52
@nking-1
Copy link
Contributor Author

nking-1 commented Nov 22, 2022

Here's the gist of the logic flow of how data is sent to the server, and how it's received and put in the Redux store.

adatest_state_rework

Copy link
Contributor

@slundberg slundberg left a comment

Choose a reason for hiding this comment

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

looked very quickly and just had one issue to flag on the local model.

@@ -157,8 +157,9 @@ def __init__(self, topic, test_tree):
else:

# we are in a highly overparametrized situation, so we use a linear SVC to get "max-margin" based generalization
self.model = CVModel()
self.model.fit(embeddings, labels)
self.model = ConstantModel(1.0)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this switched? it seems like it will disable all topic learning :)

Copy link
Contributor Author

@nking-1 nking-1 Dec 6, 2022

Choose a reason for hiding this comment

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

Glad you found this, I was getting errors there for some reason so I made that change to temporarily unblock my work. I shouldn't have checked it in though. I'll revert it and double check everything still works.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@slundberg Reverted and tested again with no issues. Thanks!

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.

2 participants