-
Notifications
You must be signed in to change notification settings - Fork 336
Run Standard.Snowflake in dual JVM mode
#14474
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
Conversation
|
There is currently 40 failures: need to be addressed. |
|
|
14 failures remaining: |
distribution/lib/Standard/Database/0.0.0-dev/src/Internal/Statement_Setter.enso
Outdated
Show resolved
Hide resolved
Standard.Snowflake in dual JVM mode
Standard.Snowflake in dual JVM modeStandard.Snowflake in dual JVM mode
|
With e59ef9c we are down to 10 failures: |
| f = case Environment.get "ENSO_CLOUD_CREDENTIALS_FILE" of | ||
| Nothing -> File.home / ".enso" / "credentials" | ||
| path -> File.new path | ||
| Authentication_Service.log_message level=..Warning "credentials_file is "+f.to_text |
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.
Why Warning ? That feels more like Trace or Debug at most.
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.
- having access to logs with detailed and related information about a CI test failure is the motivation
- I needed to see these messages in the output of failed CI run.
- for example here in this run
- I don't think there is a way to see
TraceorDebugmessages for failed tests, right? - Thus
TraceorDebugwasn't enough and I had to useWarning
| _ -> | ||
| type_name = (Meta.type_of x).to_text | ||
| case type_name of | ||
| "java.math.BigDecimal" -> |
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.
Isn't this a bug in pattern matcher if something that is "java.math.BigDecimal" doesn't match the previous _: BigDecimal case?
If it is some kind of a special case it deserves some comment at least.
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.
- here is the "motivation" 3907ebf
- I wish we could come up with some better way to deal with this obstacle!
- preferably to not leak
java.math.BigDecimalto Enso code from a different library- for example by using Pair Integer Integer
- such a library may be using "other JVM"
- when something like that happens, the Java type is different to the value's one
- and the
case ofbranch check fails
- is something like that possible, @jdunkerley?
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.
Pretty sure the "java.math.BigDecimal" pattern will be the source of bugs
Exactly! Just like any other attempt to exchange Java objects between libraries when running in Dual JVM mode. However that's not something I can fix myself. It needs grand refactoring by the libraries team and that's gonna take time.
Ironically, the more bugs we have, the more incentive there's going to be to redesign the current system built in days when "Java know no borders".
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.
- another
BigDecimalfrom other JVM problem: https://github.com/enso-org/enso/pull/14607/files#r2680897066
jdunkerley
left a comment
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.
Mostly some nits to change to keep stuff consistent with libs code then we can get this is in and see what unravels!
distribution/lib/Standard/Base/0.0.0-dev/src/Enso_Cloud/Internal/Authentication.enso
Outdated
Show resolved
Hide resolved
std-bits/table/src/main/java/org/enso/table/data/table/Column.java
Outdated
Show resolved
Hide resolved
std-bits/base/src/main/java/org/enso/base/enso_cloud/CloudAPI.java
Outdated
Show resolved
Hide resolved
| var localStorage = | ||
| switch (localType) { | ||
| case BooleanType type -> BoolBuilder.fromAddress(size, data, validity).seal(storage); | ||
| case BooleanType _ -> BoolBuilder.fromAddress(size, data, validity).seal(storage); |
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.
The _ variable has caused issues with debugging in the past which is why we avoid due to the targetJavaVersion being locked to 17.
| case BooleanType _ -> BoolBuilder.fromAddress(size, data, validity).seal(storage); | |
| case BooleanType type -> BoolBuilder.fromAddress(size, data, validity).seal(storage); |
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.
- what kind of issue?
- wasn't that caused by previous version of Frgaal?
- last updated by Updating to Frgaal 24.0.0 #13621
- how can I verify we still need to avoid using
_? - related usage of _ to address together with this inquiry
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.
Only blocks JUnit work in IntelliJ. And there are other issues that were merged over the last few weeks.
This can be worked around by changing the targetJavaVersion so this isn't a blocker for anyone who isnt me
std-bits/table/src/main/java/org/enso/table/data/column/builder/Builder.java
Show resolved
Hide resolved
std-bits/table/src/main/java/org/enso/table/data/column/builder/Builder.java
Outdated
Show resolved
Hide resolved
7f6ba8c to
1bbb22a
Compare
Co-authored-by: James Dunkerley <[email protected]>
| private val macX64Release = NativeImageSize(200, 457) | ||
| private val macARM64Release = NativeImageSize(200, 473) | ||
| private val testNISize = NativeImageSize(100, 592) | ||
| private val windowsX64Release = NativeImageSize(200, 390) |
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.
- Specifies new sizes
- see also Reduce expected NI sizes #14565
|
Jaroslav Tulach reports a new STANDUP for yesterday (2026-01-05): Progress: .
|
| get_big_decimal (that : Decimal) = that.big_decimal | ||
| Returns two numbers: the actual value and the scale associated to it | ||
|
|
||
| value_with_scale (that : Decimal) -> Pair Integer Integer = |
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.
- This is the safe way to transfer
BigDecimalbetween two JVMs Pair Integer Integeris a regular Enso atom- with either
longorBigIntegerfields - both those types are transferable between the JVMs
- with either
- atom instance is "transferable" as well
| return new CloudAPI(apiRootUri, cloudProjectId, cloudSessionId); | ||
| } | ||
|
|
||
| public static void flushCloudCaches() { |
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.
- this method should be removed by Flush system caches via
Runtime.gc#14557 - those who call this method should call
Runtime.gc flush_caches=True - the
cachedfield in this class should useManaged_Resource - such a resource will be cleared by the system when caches are flushed
| @@ -1,4 +1,5 @@ | |||
| from Standard.Base import all | |||
| import Standard.Base.Runtime.Ref.Ref | |||
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.
- I don't like this
Ref.Refduplication - it is needlessly verbose
- I'd like to change the code to allow just
import Standard.Base.Runtime.Ref - opinions?
|
hubertp
left a comment
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.
Pretty sure the "java.math.BigDecimal" pattern will be the source of bugs as it is easy to miss in the grand scheme of things that some values might be coming from other JVM and don't match on the regular type.
But I'm not going to block the PR because of this concern.
|
Jaroslav Tulach reports a new STANDUP for yesterday (2026-01-06): Progress: .
|
Pull Request Description
ensonative image executable.Important Notes
Execute in mock dual JVM mode via:
Checklist
Please ensure that the following checklist has been satisfied before submitting the PR:
Java,
Standard.Testpendingfield lazy #14536 merged in