duck7000 / imdbGraphQLPHP

5 stars 0 forks source link

Any interest / benefit in reducing number of GraphQL API calls required by combining similar queries? #64

Open mosource21 opened 3 days ago

mosource21 commented 3 days ago

The library (title.php) currently uses many calls to the IMDB API.

All these separate GraphQL API calls are relatively slow and "costly" on resources.

Is there any interest / benefit on perhaps trying to reduce the number of API calls?

Proposal 1 Is is possible to combine the GraphQL for the "mainRating" and "Metacritic" into a single GraphQL query and get both values with a single API call?

Thanks

duck7000 commented 3 days ago

Well yes that is a possibility. The slow response is mostly the waiting time for the api call to be executed is my understanding. The execution of the api call itself is quick (for the smaller methods)

I made all methods separate to be able to use what everybody exactly want without collecting lots of data that is not been used. On the other side combining methods reduces API calls, but we/i have to be aware that the API query's not going to be too complicated to understand.

So for example combining all methods would be a impossible task as the query and the method would become so large.

Combining smaller methods with a small query would be a option as long as they somehow belong together

So @mosource21 would you do some more proposal's?

Maybe mainRating, Metacritic and Votes could be combined? they all belong to "rating stuff"

mosource21 commented 2 days ago

Well yes that is a possibility. The slow response is mostly the waiting time for the api call to be executed is my understanding. The execution of the api call itself is quick (for the smaller methods) we/i have to be aware that the API query's not going to be too complicated to understand. So for example combining all methods would be a impossible task as the query and the method would become so large.

Yes completely agree it is a balancing act. I wouldn't say it is slow at the moment it just could be much more efficient if we think there it is going to be a benefit. Everyone is using the library different but I could be using 10 API calls per title to only get back a relative small amount of data.

Combining smaller methods with a small query would be a option as long as they somehow belong together Maybe mainRating, Metacritic and Votes could be combined? they all belong to "rating stuff"

Yes that sounds ideal. All assuming GraphQL can get all three items together?

So @mosource21 would you do some more proposal's?

These are specific to my use and exposure of all the library functions is limited so I may not have a great overview. Not sure if they will meet any criteria you are setting for "belonging together". If the method is returning "complex / multi fielded" data I thought it best to just leave as it is but anything which is just a single value or a list of single values I thought was fair game.

Proposal 2 Add "meta { canonicalId }" to titleYear(). Having the expense of an API call for just the canonicalId via checkRedirect seems excessive and wanting to know the canonicalid is something you would probably also want to know just like Titles, Year etc? Have a $this->imdbCanonicalID property that the end developer can check if they want to (set via titleYear and checkRedirect)?

Proposal 3 keyword(), language() and genre() These are all simple lists of basic general data if you get one of them you may as well get the others - you probably need them anyway?

Try It First Before Investing To Much Before going to far down the path I was thinking try it with "rating stuff" though. Are you going to try to keep compatibility or just change completely how it works? Some of the methods are currently returning values direct from the queried data and not $this->property so care is needed.

As before more than happy to be involved in the process if you want input, code suggestions and help with testing etc.

duck7000 commented 2 days ago

Thanks for your input, i'l take all this into consideration.

I'll check first if those three even can be combined at all

@GeorgeFive what is your view on this?

GeorgeFive commented 2 days ago

I don't think it's a bad idea. I'd like to keep compatibility of course, and the one thing I do enjoy now is how easy it is to find something. Could that be kept when combining? For example, if I want to get languages... I really don't even need the wiki or docs or anything, I can just look at $langs. Plot? It's over there in... dun dun dun... $plot.

If we start combining stuff, ie, $grading contains rating, metacritic, votes, etc... that may make it more difficult to find stuff.

I also happen to enjoy how everything is spread out, because I can get exactly what I need and nothing else. For example, I actually do get keywords, but not language and genres.

I don't think this is a HUGE issue, but it's just something to keep in mind.

duck7000 commented 2 days ago

Mm still thinking about this

Combining wouldn't much help i guess, let me give a example for my use case if combining rating, metacritics and votes this will output an associative array with values. In my program i use those values separately so i have to call the same function over and over again thus still making the same api calls

duck7000 commented 2 days ago

Thanks @GeorgeFive

I also enjoy that everything is split out, so everybody can use exactly what they need. And combining can make that method slower as there is more data to be processed. And yes compatibility is a issue after combining

So it may not be the best solution after all i guess. So sorry @mosource21 but i will keep it like it is, it works great, simple to understand and imdb GraphQL does not care about a lot of api calls, they use it on the fly on their own website.

I do appreciate you all for thinking about how to make it better though, so thank you all!

mosource21 commented 2 days ago

No problem that is why I asked. I can't argue it does work great.

I may have a try myself though is there any resources on the basics of GraphQL and specifics for IMDB (fields name list) that you use?

Can I just change the query name to whatever I like and add more fields like below and it should just work or does things like query name (NewRating in my example below) need to be something specific to IMDB to work etc?

$query = <<<EOF
    query NewRating(\$id: ID!) {
      title(id: \$id) {
        ratingsSummary {
          aggregateRating
          voteCount
        }
      }
    }
EOF;
duck7000 commented 2 days ago

Documentation is here https://developer.imdb.com/documentation/api-documentation/sample-queries/title-name/?ref_=side_nav

QueryName is indeed anything you like as long as you use the same name in api request ("TitltleYear" must be the same as queryName) $data = $this->graphql->query($query, "TitleYear", ["id" => "tt$this->imdbID"]);

You can use multiple query's like in your example. Any spaces inside the query are stripped off so don't worry about that but keep the structure humanly readable.

I really doubt combining would benefit speed but it could make some difference i suppose

If you really want to know everything about how GraphQL works you have to study al lot though, it is a lot! So easiest is to re use code that already is there and working

duck7000 commented 15 hours ago

@mosource21 @GeorgeFive I'm still thinking about this combining thing

I can for example add titleYear() to __construct() so that the basic info is always available? It still will be callable through separate methods but saves excessive calls to the same method over and over again

Example If i want title, movieType and year we (in the current state) call those separate methods each calling titleYear() 3 times for the same info, this is stupid.

We can add other basic info that anyone would want to titleYear() accessible through separate methods

So combining stuff might not a bad idea after all?

Basis info could/would be: title original title year endYear movieType language country genre plotoutline? runtime?

GeorgeFive commented 6 hours ago

Not a bad idea... it won't do much for me (this would save me two calls total), but if it helps others, I'm definitely for it.

duck7000 commented 51 minutes ago

In my case this will save a total of 9 calls as i use all. So yes it is worth the trouble i guess.

I'll leave this on the back burner for now