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

Add reduction history page to display runs #36

Merged
merged 25 commits into from
Feb 29, 2024
Merged

Conversation

Dagonite
Copy link
Collaborator

@Dagonite Dagonite commented Jan 18, 2024

Closes #33.

Can now view runs via the reduction history page.

How to test

Get a populated database connected to the API. Run the API with uvicorn ir_api.ir_api:app --host 0.0.0.0 --port 8080 on the implement_specification_pattern branch. Launch the plugin and see if ir/reduction-history/{instrument} is populated with runs for each instrument.

Build the frontend repo and test when ran through SciGateway.

@Dagonite Dagonite requested a review from Pasarus January 18, 2024 13:06
Copy link
Member

@Pasarus Pasarus left a comment

Choose a reason for hiding this comment

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

Ok so I made a quick change to solve the issue that we are presently unable to define where the API URL. This can now be set in a new .env file, with a default here using localhost:8000 as per the code submitted.

Locally I am using prod as the requests here should not impact the contents of the data.

For the actual issues with this web page:

  • You cannot sort by RBNumber, Run Start time, Run End time or Run Output. This returns us wildly not useful data on the front page and we should default to sorting by RBNumber or probably Reduction ID in an extra column as that was added to the DB most recently? as that will be the most useful information for a user to see. Every column should be sortable on this page.
  • I am not convinced the Run Output is correct. It's missing more than one file in any scenario? it seems to be the run input file. Come to think of it should be a separate column just after RB Number, these 2 columns can be quite thin.
  • Would it be possible to follow this: https://stackoverflow.com/a/66817589 it would be great to alternate colours on the table to ensure that a user can easily follow data along and improve readability.
  • The page takes a very long time to inform the user it is doing anything when switching from one instrument to another. It just takes a long time to do anything with the IR-API so I'll have a chat with Keiran tomorrow to see what he thinks might be the issue.

@Dagonite
Copy link
Collaborator Author

Dagonite commented Jan 18, 2024

You cannot sort by RBNumber, Run Start time, Run End time or Run Output

Is it even possible to sort by these with the way the API is? From my understanding and implementation right now, one fetch request needs to be made just to return a single run. So sorting would have to be done on the frontend because the API doesn't offer this functionality.

I am not convinced the Run Output is correct. It's missing more than one file in any scenario?

I can only see one "Run Output" file per run. I don't think the db_generator script generates runs with multiple of these files unless I'm mistaken?

Would it be possible to follow this: https://stackoverflow.com/a/66817589 it would be great to alternate colours on the table to ensure that a user can easily follow data along and improve readability.

Will do 👍

The page takes a very long time to inform the user it is doing anything when switching from one instrument to another. It just takes a long time to do anything with the IR-API so I'll have a chat with Keiran tomorrow to see what he thinks might be the issue.

Is relatively quick on my end but it might be because of my hardware. But yeah the short delay is the API fetching 50 (think is the default I set it to) reductions, then counting how many reductions the specific instrument has to determine pagination, and then getting runs for those 50 reductions using their IDs. So it's doing 52 fetch requests per page because I see no other way to do it.

@Pasarus
Copy link
Member

Pasarus commented Jan 19, 2024

Is it even possible to sort by these with the way the API is? From my understanding and implementation right now, one fetch request fetch needs to be made just to return a single run. So sorting would have to be done on the frontend because the API doesn't offer this functionality.

If you need increased functionality to provide these frontend features that can be requested, please do so if you require it either by going through @keiranjprice101 or me.

I can only see one "Run Output" file per run. I don't think the db_generator script generates runs with multiple of these files unless I'm mistaken?

You are mistaken, I am using real-world data to test this.

The page takes a very long time to inform the user it is doing anything when switching from one instrument to another. It just takes a long time to do anything with the IR-API so I'll have a chat with Keiran tomorrow to see what he thinks might be the issue.

Is relatively quick on my end but it might be because of my hardware. But yeah the short delay is the API fetching 50 (think is the default I set it to) reductions, then counting how many reductions the specific instrument has to determine pagination, and then getting runs for those 50 reductions using their IDs. So it's doing 52 fetch requests per page because I see no other way to do it.

The reason why it is fast for you and not for me, is that you are pinging your local API with your local DB, I am pinging the actual in production API, with the actual in production Database with real world data.

@Dagonite
Copy link
Collaborator Author

Dagonite commented Jan 19, 2024

If you need increased functionality to provide these frontend features that can be requested, please do so if you require it either by going through @keiranjprice101 or me.

Then yes I would need this functionality otherwise all the runs would need to be fetched first before any sorting could occur.

The reason why it is fast for you and not for me, is that you are pinging your local API with your local DB, I am pinging the actual in production API, with the actual in production Database with real world data.

Could we get this set-up for me so I can be using more accurate reduction data?

@Pasarus
Copy link
Member

Pasarus commented Jan 19, 2024

Then yes I would need this functionality otherwise all the runs would need to be fetched first before any sorting could occur.

Can you detail the calls you want to make? and the expected output

Could we get this set-up for me so I can be using more accurate reduction data?

I've messaged you on teams.

@Dagonite
Copy link
Collaborator Author

Can you detail the calls you want to make? and the expected output

Something like these with sorting query strings?


  1. /instrument/{instrument}/runs

E.g:

[
  {
    "filename": "/archive/NDXHIFI/Instrument/data/cycle_19_02/HIFI64374.nxs",
    "experiment_number": 64374,
    "title": "Remember professional outside young both environment discover hope official.",
    "users": "Connie Brown, George Ruiz",
    "run_start": "2021-03-08T07:41:16",
    "run_end": "2021-03-08T08:13:16",
    "good_frames": 7795,
    "raw_frames": 9514,
    "instrument_name": "HIFI"
  }
]

  1. /instrument/runs same as above but returns all runs and isn't instrument specific

  1. /instrument/{instrument}/runs/count

E.g:

{
  "count": 300
}

  1. /instrument/runs/count

E.g:

{
  "count": 10000
}

@keiranjprice101
Copy link
Collaborator

@Dagonite
So I will add the following. Let me know if this is complete

/runs returns all runs across all instruments, with limits offsets and order_bys like reductions
/runs/count same as above but count only
/instrument/<instrument>/runs runs for instrument

@Dagonite
Copy link
Collaborator Author

@keiranjprice101

Yes, looks right.

@Dagonite Dagonite requested a review from Pasarus January 25, 2024 14:27
@Dagonite
Copy link
Collaborator Author

When testing this PR I assume you won't be able to do it using the non-localhost API because it doesn't have the newer endpoints added.

@Pasarus
Copy link
Member

Pasarus commented Jan 29, 2024

When testing this PR I assume you won't be able to do it using the non-localhost API because it doesn't have the newer endpoints added.

We will be able to once this PR is merged: fiaisis/FIA-API#251

I will get to reviewing PRs when I can, at the moment I have none-project work that needs to be completed.

@Pasarus Pasarus merged commit a1865b3 into main Feb 29, 2024
2 checks passed
@Pasarus Pasarus deleted the reduction-history-page branch February 29, 2024 15:02
Dagonite added a commit that referenced this pull request Mar 6, 2024
commit 6de3801
Author: Samuel <[email protected]>
Date:   Wed Mar 6 09:59:00 2024 +0000

    Add logo design file (#55)

commit 6df5887
Author: Samuel <[email protected]>
Date:   Wed Mar 6 09:58:39 2024 +0000

    Make plugin url an environment variable (#54)

    * Move plugin url into environment var

    * Update explanation comment

commit 0f401c0
Author: Samuel <[email protected]>
Date:   Tue Mar 5 09:03:13 2024 +0000

    Add initial Cypress tests (#38)

    * Update route names

    * Update reduction history page

    * Remove commented out code

    * Remove redundant comments

    * Allow a more dynamic selection of the IR-API IP using an env file, this way staging and prod can be different and we can also just change it for whatever we need during development.

    * Alter fetch request logic

    Add accessibility changes to table

    * Update reduction history button url

    * Correct route for SciGateway sidebar

    * Alter homepage browse reductions url

    * Add missing new line

    * Use different port

    Avoids issues when running the app through SciGateway

    * Add missing table headings

    Some headings currently have placeholder values until the API functionality is added

    * Fix url

    * Make even rows background color use theme

    * Delete History.tsx

    * Sort by desc initially so recent reductions at the top

    * Correct wrong run date being rendered

    * Add initial cypress tests

    * Disable failed test

    * Comment out broken up code

    * Update TODO comment

    * Remove duplicate route

    * Update yarn.lock

    Accounts for missing dependencies

    * Delete cypress/fixtures/example.json

    * Remove false test

    ---------

    Co-authored-by: Samuel Jones <[email protected]>
    Co-authored-by: Samuel Jones <[email protected]>

commit 327f7ce
Author: Samuel <[email protected]>
Date:   Fri Mar 1 13:14:40 2024 +0000

    Add Interactive Reduction logo and fix light / dark mode bug (#43)

    * Update route names

    * Update reduction history page

    * Remove commented out code

    * Remove redundant comments

    * Allow a more dynamic selection of the IR-API IP using an env file, this way staging and prod can be different and we can also just change it for whatever we need during development.

    * Alter fetch request logic

    Add accessibility changes to table

    * Update reduction history button url

    * Correct route for SciGateway sidebar

    * Alter homepage browse reductions url

    * Add missing new line

    * Use different port

    Avoids issues when running the app through SciGateway

    * Add missing table headings

    Some headings currently have placeholder values until the API functionality is added

    * Fix url

    * Make even rows background color use theme

    * Delete History.tsx

    * Sort by desc initially so recent reductions at the top

    * Correct wrong run date being rendered

    * Add svg logos

    * Add wordless logos

    * Update favicon.ico

    * Move theme out of component

    Fixes bug where pages weren't re-rendering elements when switching between pages

    * Minify svg

    Removes incompatible tags

    * Add IR logo to top of web app

    * Use more appropriate file names

    * Use updated logo design

    Made the text and logo much more legible

    ---------

    Co-authored-by: Samuel Jones <[email protected]>
    Co-authored-by: Samuel Jones <[email protected]>

commit a1865b3
Author: Samuel <[email protected]>
Date:   Thu Feb 29 15:01:43 2024 +0000

    Add reduction history page to display runs (#36)

    * Update route names

    * Update reduction history page

    * Remove commented out code

    * Remove redundant comments

    * Allow a more dynamic selection of the IR-API IP using an env file, this way staging and prod can be different and we can also just change it for whatever we need during development.

    * Alter fetch request logic

    Add accessibility changes to table

    * Update reduction history button url

    * Correct route for SciGateway sidebar

    * Alter homepage browse reductions url

    * Add missing new line

    * Use different port

    Avoids issues when running the app through SciGateway

    * Add missing table headings

    Some headings currently have placeholder values until the API functionality is added

    * Fix url

    * Make even rows background color use theme

    * Delete History.tsx

    * Sort by desc initially so recent reductions at the top

    * Correct wrong run date being rendered

    * Increase prettier line length

    * Refactor to accommodate api changes

    * Adding missing sorting functionality

    * Change any type

    * Prettier formatting changes

    * Remove debugging logs

    * Add reduction status formatting

    * Add missing sorting functionality

    ---------

    Co-authored-by: Samuel Jones <[email protected]>
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.

Add reduction history page
3 participants