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

[Draft] Github CI, DSL Builder, Seperate logging factory #251

Closed
wants to merge 31 commits into from

Conversation

smallshen
Copy link
Contributor

No description provided.

@smallshen
Copy link
Contributor Author

@zaleslaw zaleslaw added this to the 0.4 milestone Oct 4, 2021
@zaleslaw
Copy link
Collaborator

zaleslaw commented Oct 4, 2021

Looks interesting, thanks for this PR, I need a time to check on my fork and return with the results

@zaleslaw zaleslaw added the Review This PR is under review label Oct 4, 2021
@V3lop5
Copy link

V3lop5 commented Oct 7, 2021

Hi @smallshen @zaleslaw,

I would like to see Gradle wrapper validation in use.
https://github.com/gradle/wrapper-validation-action

This small action validates, thats the checked in gradle-wrapper.jar was created by Gradle. It takes nearly no time and provides security.

@smallshen I already created a PR in your fork. smallshen#1
Thanks!

Added gradle wrapper validation
@smallshen
Copy link
Contributor Author

@V3lop5 thanks, merged.

@smallshen
Copy link
Contributor Author

@zaleslaw
I saw the conflicts on onnx/build.gradle
It is caused by Gradle version, newer version of Gradle use implementation

@smallshen
Copy link
Contributor Author

In the Kotlin compiler, lambda is compiled into an object (without compiler flags).
oshai/kotlin-logging#34
And this causes performance issues

Plus the recent log4j RCE has made more people change logging frameworks, (for example, in my organization we are not allowed to use Apache related frameworks).
This implementation is lightweight and easy to extend without any loss of functionality.

@zaleslaw
Copy link
Collaborator

Hi, @smallshen you did a really great job, but this migration will be postponed for a few months to clarify the situation on the market among our users who embed the KotlinDL functionality in their apps.

  1. Log4j/logback and others is popular among Java/Kotlin backend developers and have widely known configuration
  2. Replacement of the logging framework could lead to new bugs and CVE but they will be not found so fast as in the log4j case
  3. In a framework like KotlinDL we will be not affected by performance issues due to a very tiny percentage time of logging methods execution in comparison to tensor calculations

But I suggest keeping this PR (probably marked as draft or something else) to revisit the problem in the next two months.

Now it contains a lot of mixed proposals (Gradle update (it's done), Github CI, DSL for building neural networks (we have a separate PR for that as I remember), and a proposal for a new logging module (which is postponed))

@smallshen
Copy link
Contributor Author

Log4j/logback is supported with Log4JFactory.setup(), and users don't need to change anything on their configs

@smallshen
Copy link
Contributor Author

But I suggest keeping this PR (probably marked as draft or something else) to revisit the problem in the next two months.

Now it contains a lot of mixed proposals (Gradle update (it's done), Github CI, DSL for building neural networks (we have a separate PR for that as I remember), and a proposal for a new logging module (which is postponed))

I agree with this

@smallshen smallshen changed the title Update Gradle Version and Add Github Workflow CI/ [Draft] Github CI, DSL Builder, Seperate logging factory Dec 16, 2021
@smallshen
Copy link
Contributor Author

@zaleslaw

Log4j/logback and others is popular among Java/Kotlin backend developers and have widely known configuration

Creating loggers is handled internally by the log4j API, so the logging behavior stays the same using Log4JFactory and does not require any changes.

https://github.com/smallshen/KotlinDL/blob/master/logging/logging-log4j/src/main/kotlin/org/jetbrains/kotlinx/dl/logging/log4j/ Log4JFactory.kt

and everything is handled by log4j's internal logger.

https://github.com/smallshen/KotlinDL/blob/master/logging/logging-log4j/src/main/kotlin/org/jetbrains/kotlinx/dl/logging/log4j/ LoggerWrapper.kt#L30

@zaleslaw zaleslaw modified the milestones: 0.4, 0.5 Dec 16, 2021
# Conflicts:
#	api/src/main/kotlin/org/jetbrains/kotlinx/dl/api/core/GraphTrainableModel.kt
@zaleslaw zaleslaw removed this from the 0.5 milestone Apr 19, 2023
@smallshen smallshen closed this Jun 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Review This PR is under review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants