-
Notifications
You must be signed in to change notification settings - Fork 13
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
feat: trending queries #301
Conversation
* chore: preps makefile for testing * feat: randomness reduced on miner selection and query * fix: quotes on keyword * fix: cleanup makefile * fix: centralize volume testing window param * feat: tweets by UID (miner) (#303) * fix: similarity score and prep makefile for testing * fix: improvement on unique tweet ids * fix: stores unique tweets by uid * fix: removes unique tweets for uid when they are de-registered * fix: simpler tweet indexing * fix: adds endpoint to expose tweets by uid as well * fix: function name change * fix: cleanup endpoint logic * fix: cleanup notes * fix: remove note * fix: revert makefile * fix: makefile
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #301 +/- ##
===========================================
+ Coverage 53.18% 65.47% +12.28%
===========================================
Files 23 23
Lines 1286 1318 +32
===========================================
+ Hits 684 863 +179
+ Misses 602 455 -147 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, tested, seems to be working as expected ( only on testnet )
{ | ||
"mainnet": { | ||
"organic": { | ||
"sample_size": 3, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would that make sense that the sample size of the organic is defined by the requester instead of the miner?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can definitely make a ticket for that - my concern is that there would need to be a hard-coded limit on the organic sample size, otherwise someone could spam the entire network and request 240 miners / call
waiting for tests to pass / codcov report, and community input on mip2 before merging! |
…crets, among other things (#306) * setup hotkey of test miner * add async library, fix warnings * add async library, fix warnings * add async library, fix warnings * fix validator tests. --------- Co-authored-by: JD <john@masa.ai>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
One comment: I am facing some weird errors when getting 429 or similar from the protocol ( They are not clear enough I guess )
2024-11-20 10:30:07.382 | ERROR | bittensor:loggingmachine.py:457 | - ConnectionError#af67eeea-81d9-429e-88a6-1c44fbee9f1b: HTTPConnectionPool(host='x.x.x.x', port=8080): Max retries exceeded with url: /api/v1/data/twitter/tweets/recent (Caused by NewConnectionError('<urllib3.connection.HTTPConnection object at 0x10f5ca210>: Failed to establish a new connection: [Errno 61] Connection refused')) -
Maybe we could improve this error handling and exposing something like "4XX error from protocol" to make that easier to understand
@hide-on-bush-x added improved error handling in recent tweets request! |
* chore: installs sdk 0.2.5 locally, preps branch * feat: incorporates new sdk, removes count and twitter config, updates query to since yesterday * fix: timedelta * fix: validation function name * feat: miners define max count * fix: cleans up timeout and counts * feat: reduce randomness (#302) * chore: preps makefile for testing * feat: randomness reduced on miner selection and query * fix: quotes on keyword * fix: cleanup makefile * fix: centralize volume testing window param * feat: tweets by UID (miner) (#303) * fix: similarity score and prep makefile for testing * fix: improvement on unique tweet ids * fix: stores unique tweets by uid * fix: removes unique tweets for uid when they are de-registered * fix: simpler tweet indexing * fix: adds endpoint to expose tweets by uid as well * fix: function name change * fix: cleanup endpoint logic * fix: cleanup notes * fix: remove note * fix: revert makefile * fix: makefile * fix: increased test coverage * fix: cleans up tests * fix: makefile * fix: pushing config * fix: backup fetch * fix: naming * fix: config and mapping * fix: removes mock * fix: config for network * chore: ports entire config * fix: adds tests * fix: reverts makefile * fix: refactor forwarder * fix: since * fix: update config * fix: tempo blocktime * fix: test error string * fix: bump version * fix: more tests * fix: scoring test * fix: cleanup unused files like docker and scrips * fix: remove miner test for protocol * feat: adds max tweet count and refactors synapses * fix: healthcheck import * fix: import * fix: reverts makefile for deploy * fix: Fix tests by adding registered miner and validator wallets as secrets, among other things (#306) * setup hotkey of test miner * add async library, fix warnings * add async library, fix warnings * add async library, fix warnings * fix validator tests. --------- Co-authored-by: JD <john@masa.ai> * fix: vali test * fix: twitter limit and makefile logging * fix: state loading guards * fix: improved error handling in recent tweets * fix: adds requests library for better error handling --------- Co-authored-by: J2D3 <156010594+5u6r054@users.noreply.github.com> Co-authored-by: JD <john@masa.ai>
Description
This PR fixes #297, #298
Refactoring and Code Organization:
synapses
directory for better organization, as they are used by both validator and miner components.Enhancements in
masa/base/miner.py
:forward_tweets_synapse
inBaseMinerNeuron
to handle the forwarding of recent tweets using theRecentTweetsSynapse
class.forward_tweets_synapse
method.Updates in
masa/miner/masa_protocol_request.py
:timeout
parameter to theget
andpost
methods to allow customizable request timeouts.Improvements in
masa/miner/twitter/tweets.py
:RecentTweetsSynapse
class to include an optionaltimeout
attribute.forward_recent_tweets
function to accept amax
parameter, which is used to limit the number of tweets fetched.TwitterTweetsRequest
class to use themax_tweets
parameter from the miner configuration.Configuration Changes in
masa/utils/config.py
:--twitter.max_tweets_per_request
to define the maximum number of tweets to scrape per request.Enhancements in
masa/validator/forwarder.py
:fetch_twitter_config
method withfetch_twitter_queries
to fetch trending queries instead of a static configuration.TweetValidator
instance for validating tweets.Dependency Update in
pyproject.toml
:masa-ai
dependency version from0.2.3
to0.2.5
.These changes aim to improve the flexibility, organization, and functionality of the codebase, particularly in handling Twitter data and configuration management.
Notes for Reviewers
None
Signed commits