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

Flask Upgrades #452

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Flask Upgrades #452

wants to merge 1 commit into from

Conversation

nealmick
Copy link

@nealmick nealmick commented Nov 6, 2024

This branch contains updates for the flask web server and adds functionality for team rosters as well as player details including injury status. The main changes include 2 new templates player_modal.html and team_modal.html, these are linked to the main index.html using jinja template {% include %} feature. The flask app has been updated with new routes /player-stats/<player_id>/ and /team-data/<team_name>/ which are used to request and form the data used in the new templates.

This pr should be easier to review as its mostly HTML templates, with a few additions to the flask app. I did have some trouble with the NN runner, and had to revert it to an older version due to TensorFlow mismatch with the saved model H5 legacy format weights, however i can remove those changes as well if needed.

379325760-df346cdf-773d-42e6-8ff0-0be72732216b.mp4

@XZoro-404
Copy link

@nealmick I truly believe you are currently carrying this project since last year. I really appreciate what you are doing.
Currently I have no experience in machine learning but have been following this project in hopes to help with it one day.

@kyleskom
Copy link
Owner

I love the looks of this one! I have been out on travel and have been sick as well, I promise to get to this pr this week!

@nealmick
Copy link
Author

okay appreciate it, let me know if you want any changes

@XZoro-404
Copy link

I'm not sure if it has already been done or not but when I get home I will bug check how many times the api gets called. Because if it gets called every time there is a page reload then with the free api key there is an average of 11.11 reloads a day (considering that there is 3 different api calls).
That said I believe there should be a button that will pull new api data.

I have some thoughts on how to successfully do this.

@nealmick
Copy link
Author

You are correct nothing is cached between page reloads. I do have a subscription attached to the key with a higher rate limit, it has 1k request/day, I use this key for other projects too. Currently the api usage is at 2.8% for the month. Caching would definitely help reduce the api hits and potential issues with rate limit. When the page first loads it shouldn't make any extra requests, only when clicking on teams it triggers the team roster request, and then clicking on a player will hit the 2 player endpoints. Adding cache is a good idea, i tend to use the python pickle library for saving and caching. I would use the team ABV and playerID as cache keys. The only issue with caching is stale data and handling cache invalidation. I think the simplest solution would be like a clear cache button as you said, but if someone docent understand to clear the cache they could look at stale data and may think that's an issue. Caching would reduce api hits and loading time for the team and player modals which would be good.

I think caching would add more complexity for this pr, I would probably hold off and add caching later as long as we are still within api limits.

@XZoro-404
Copy link

To ensure that there is no abuse with your api key I would exclude it personally. But that is all up to you.

I agree that caching is outside of the scope of this pr.

But to delve a little more into the topic. I think the way to cache it would include the date and time that it was cached. Then when requesting team/player stats it will compare the current date with the cached date (this would make it so the users would not have to refresh the data daily). Then there could be a button that could do one of the following:

  1. Clear all of the current days cached data
  2. Recursively clear the data from users current location ie. if on the Bulls team page and clicks refresh button only clears the data of the bulls team and player data
  3. Pull all new data (do all api calls for all teams/players)

With that said in terms of holding onto previous cached data we could either hold all data for the entire season or we could only hold it for x amount of days.

@nealmick
Copy link
Author

Yea there is some potential for abuse, but its a hard rate limit im not too worried, can always rotate keys if needed. Caching is definitely a good idea, im not sure how much it would help if we expire cache daily or even longer then it will get cache miss on each clients first requests. Also the cache would not be shared between individual installs if its done locally. Maybe could setup a remote server that would cache for all installs and remove rate limit completely, or rate-limit individual clients.

@XZoro-404
Copy link

I'm not sure which api subscription you have but if it is the 'PRO' version then there is not absolute hard cap by default.
`Basic: This is a free plan offering a hard cap of 1,000 hits per month. Perfect for testing. This plan does NOT require a credit card.

Pro: $10/month - 1,000 requests per day with $0.01 each addl request.`
I don't mean to harp on this to much just want to point it out

how would it get a cache miss on clients first request? The request should be its own function and should be called on page load and compare against the already cached data's date.

A remote server is an interesting idea, but I believe it would go against the nature of open source. For security reasons it would introduce an attack vector. Not only that but if we introduce a way for users to update the cache throughout the day then it would change it for everyone.

@nealmick
Copy link
Author

No problem, I do have a custom plan with a hard limit and no extra overage, its the same as the 10$ just with hard limit.

I think the cache mainly would help if someone loads the player or team data multiple times, however after 24 hours there could be new games or updated injury status so i think invalidation is good idea. However this means the cache would only cover reloads of the same data within that 24 hour period. Usually first loads are cache misses, unless the data is pre fetched or cache is updated regularly like every 24h, but that would need requests for every player every 24 hours and for each user separately.

I appreciate the input, security could be an issue but if you sanitize the data and do everything properly it should be okay, however adding remote server would add dependency on external service and I get it, thats not really opensource. However i do think it would be nice to have a public hosted version of the app, with like default models so people can just click a link and check it out, but you know it all depends on where this project is going... and like you said in discussions there inst really a roadmap and so forth.

@XZoro-404
Copy link

for every player every 24 hours and for each user separately this would only be a concern if everyone was using the same api key. (correct me if I'm incorrect on that)
cache is updated regularly like every 24h This would be the idea I believe.

I am 100% on board with a public hosted version of the app and has been something I was looking into today. Though I believe that there is actually a google sheet that is updated daily with the picks (not sure if it is still around). I think this project has been in kind of a slump for a while with it being updated about once a year. But really the only person who can change that as of right now is @kyleskom

Copy link
Author

Choose a reason for hiding this comment

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

includes update to fix legacy model format error.

@nealmick
Copy link
Author

@kyleskom any update on getting this reviewed?

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.

3 participants