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.

Sign up for GitHub

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

Jump to bottom

Code clean-up for release #36

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

Code clean-up for release #36

wants to merge 1 commit into from

Conversation

jeofo
Copy link
Member

@jeofo jeofo commented Sep 5, 2024

  • BREAKING: update to python3.12—remove pipenv and reinstall
  • Removed scripts (@joyliu-q can we remove some k8 stuff now?)
  • Cleaned up database implementation
  • Cron job now scheduled via FastAPI Server
  • General bugfixes

@jeofo jeofo requested a review from joyliu-q September 5, 2024 18:50
@jeofo
Copy link
Member Author

jeofo commented Sep 5, 2024

TODO: Update CI/CD to 3.12

@joyliu-q
Copy link
Contributor

joyliu-q commented Sep 6, 2024

@JeffersonDing you want to try your hand at Kittyhawk (remove the K8s stuff in /k8s/main.ts)?

@jeofo
Copy link
Member Author

jeofo commented Sep 6, 2024

@JeffersonDing you want to try your hand at Kittyhawk (remove the K8s stuff in /k8s/main.ts)?

Yeah, I'll try 😔

joyliu-q
joyliu-q reviewed Sep 6, 2024
View reviewed changes
Copy link
Contributor

@joyliu-q joyliu-q left a comment

Choose a reason for hiding this comment

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

Coming along nicely! You should try your hands at the /k8s stuff, it's written in TS and meant to be easily configured.

tests/test_load.py
@@ -65,4 +65,3 @@ def test_load():
end = time.time()
runtime = end - start
print(f"Time taken: {runtime} seconds")
assert runtime < BENCHMARK_TIME
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason we removed this assertion?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll add it back, was trying to make tests pass

src/main.py
@repeat_at(cron="0 0 * * *")
async def flush_db():
count = await redis_count()
print(f"{count} items found in redis")
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of printing these, let's use Python's logging, and set an overall logger with some settings.

src/database.py
json_string = json.dumps(data)
data = json.loads(json.loads(json_string))
except ValueError as e:
print(e)
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto

@joyliu-q joyliu-q changed the title Code claenup for release Code clean-up for release Sep 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joyliu-q joyliu-q joyliu-q left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jeofo @joyliu-q