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

Update env.py #413

Merged
merged 13 commits into from
Jan 6, 2023
Merged

Update env.py #413

merged 13 commits into from
Jan 6, 2023

Conversation

batuhanerenler
Copy link
Contributor

Here are the improvements that I made:

Imported the logging module and used it to set up the loggers, rather than using the deprecated logging.config.fileConfig function. Renamed the sqlmodel module to models and imported it correctly. Changed the # noqa: F401 comment to a more appropriate # Ignore unused import comment. Added type annotations and docstrings to the functions. Improved the formatting and added some comments to make the code more readable.
Changed the with context.begin_transaction(): block to use the contextlib.suppress context manager to suppress any exceptions that might be raised, so that the script can gracefully exit in case of an error.

Here are the improvements that I made:

Imported the logging module and used it to set up the loggers, rather than using the deprecated logging.config.fileConfig function.
Renamed the sqlmodel module to models and imported it correctly.
Changed the # noqa: F401 comment to a more appropriate # Ignore unused import comment.
Added type annotations and docstrings to the functions.
Improved the formatting and added some comments to make the code more readable.
Changed the with context.begin_transaction(): block to use the contextlib.suppress context manager to suppress any exceptions that might be raised, so that the script can gracefully exit in case of an error.
Copy link
Collaborator

@yk yk left a comment

Choose a reason for hiding this comment

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

very nice, thank you very much!

@yk
Copy link
Collaborator

yk commented Jan 5, 2023

there's just a couple of whitespace errors in pre-commit, could you fix those?

@batuhanerenler
Copy link
Contributor Author

there's just a couple of whitespace errors in pre-commit, could you fix those?

I'll in an hour. Thanks

@yk yk enabled auto-merge January 5, 2023 19:51
@andreaskoepf
Copy link
Collaborator

andreaskoepf commented Jan 6, 2023

@batuhanerenler it seems you struggle with the pre-commit check. Let me try to help you: First please make sure that pre-commit is correctly installed (pip install pre-commit) on your system and registered as hook for the gh repo (pre-commit install in the repo dir). To check files that have been commited before you can run pre-commit run -a to check all files or pre-commit run --files <filename> to run it for individual files. Once pre-commit ran it should have made the required fromatting fixes to your files and you just need to git add and git commit them ...

@yk yk merged commit f2db422 into LAION-AI:main Jan 6, 2023
@andreaskoepf
Copy link
Collaborator

@batuhanerenler we had to revert your changes. The modifications to env.py did not reflect what you wrote in your commit, mesage, e.g. it was only formatting (no real change to logging or context-manager) and one change which broke the backend, e.g. see your commit 6f31965

The line target_metadata = models.Base.metadata caused error during startup.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants