[SUPPORT] java.lang.NoClassDefFoundError: org/apache/hudi/com/fasterxml/jackson/module/scala/DefaultScalaModule$ when doing an Incremental CDC Query in 0.14.1

Describe the problem you faced
When doing an Incremental CDC Query ('hoodie.datasource.query.incremental.format': "cdc"), the job crashes with the mentioned error, this only happens in 0.14.1 and not in 0.13.1 for the same dataset.
To Reproduce
Steps to reproduce the behavior:

Write a Table with CDC Enabled (Can be either MoR or CoW, happens on both)
Insert New Data (Can be upserts, inserts, or simply write in append mode)
Query the Table with in Incremental Query with CDC Enabled (Default CDC Format Before and After Image)
Crashes with mentioned error in 0.14.1, but successfully returns the appropriate Dataframe in 0.13.1 with the Schema:

root
 |-- op: string (nullable = true)
 |-- ts_ms: string (nullable = true)
 |-- before: string (nullable = true)
 |-- after: string (nullable = true)

Expected behavior
Should return the same Dataframe and not Crash in 0.14.1
Environment Description

Hudi version : 0.13.1 and 0.14.1

Spark version : 3.3

Hive version : N/A

Hadoop version : 3.3.2

Storage (HDFS/S3/GCS..) : S3 and Local MacOS File System

Running on Docker? (yes/no) : No

Additional context
This original came from us trying to run Hudi in AWS Glue. We were running Hudi 0.13.1 in Glue 4.0 without any problems but a new QA feature to test Deletes being captured was deployed, and it started causing errors of "There should be a CDC Log File", We found out by reading through the repo, this was a bug that was occurring that was reported here https://github.com/apache/hudi/issues/9987
And that it was fixed in 0.14.1. Since Glue runs with Hudi 0.13.1 and spark 3.3 in Glue 4.0. We went ahead and downloaded and manually ran the hudi-bundle3.3 for 0.14.1, since Glue 4.0 runs with 3.3. And that's when we got the mentioned error of the Missing Class. To start "narrowing down problems". I went ahead and attempted to recreate the problem locally with a smaller test sample. And was able to reproduce it exactly the same. using the HUDI Quickstart Utils.
I leave attached the exact same code that was ran.
Some Notes:

In 0.14.1 the error also occurs when using the new "hudi_table_changes" function when trying to do it with CDC

from typing import Any

from pyspark import Row
from pyspark.sql import SparkSession
from pyspark.sql.functions import col

spark = SparkSession.builder \
    .appName("Hudi Basics") \
    .config("spark.serializer", "org.apache.spark.serializer.KryoSerializer") \
    .config("spark.jars.packages", "org.apache.hudi:hudi-spark3.3-bundle_2.12:0.13.1") \
    .config("spark.sql.extensions", "org.apache.spark.sql.hudi.HoodieSparkSessionExtension") \
    .config("spark.sql.legacy.timeParserPolicy", "LEGACY") \
    .getOrCreate()

sc = spark.sparkContext

table_name = "hudi_trips_cdc"
base_path = "XXXXXX"  # Replace for whatever path
quickstart_utils = sc._jvm.org.apache.hudi.QuickstartUtils
dataGen = quickstart_utils.DataGenerator()

inserts = sc._jvm.org.apache.hudi.QuickstartUtils.convertToStringList(dataGen.generateInserts(10))

def create_df():
    df = spark.read.json(spark.sparkContext.parallelize(inserts, 2))
    return df

def write_data():
    df = create_df()

    hudi_options = {
        "hoodie.table.name": table_name,
        "hoodie.datasource.write.recordkey.field": "uuid",
        "hoodie.datasource.write.table.type": "MERGE_ON_READ",  # This can be either MoR or CoW and the error will still happen
        "hoodie.datasource.write.partitionpath.field": "partitionpath",
        "hoodie.datasource.write.table.name": table_name,
        "hoodie.datasource.write.operation": "upsert",
        "hoodie.table.cdc.enabled": "true",  # This can be left enabled, and won"t affect anything unless actually queried as CDC
        "hoodie.datasource.write.precombine.field": "ts",
        "hoodie.upsert.shuffle.parallelism": 2,
        "hoodie.insert.shuffle.parallelism": 2
    }

    df.write.format("hudi") \
        .options(**hudi_options) \
        .mode("overwrite") \
        .save(base_path)

def update_data():
    updates = quickstart_utils.convertToStringList(dataGen.generateUpdates(10))
    df = spark.read.json(spark.sparkContext.parallelize(updates, 2))
    df.write \
        .format("hudi") \
        .mode("append") \
        .save(base_path)

def incremental_query():

    ordered_rows: list[Row] = spark.read \
        .format("hudi") \
        .load(base_path) \
        .select(col("_hoodie_commit_time").alias("commit_time")) \
        .orderBy(col("commit_time")) \
        .collect()

    commits: list[Any] = list(map(lambda row: row[0], ordered_rows))
    begin_time = commits[0]

    incremental_read_options = {
        'hoodie.datasource.query.incremental.format': "cdc",  # Uncomment this line to Query as CDC, crashes in 0.14.1
        'hoodie.datasource.query.type': 'incremental',
        'hoodie.datasource.read.begin.instanttime': begin_time,
    }

    trips_incremental_df = spark.read \
        .format("hudi") \
        .options(**incremental_read_options) \
        .load(base_path)

    # Error also occurs when using the "from_hudi_table_changes" in 0.14.1
    # sql_query = f""" SELECT * FROM hudi_table_changes ('{base_path}', 'cdc', 'earliest')"""
    # trips_incremental_df = spark.sql(sql_query)

    trips_incremental_df.show()
    trips_incremental_df.printSchema()

if __name__ == "__main__":
    write_data()
    update_data()
    incremental_query()

# Source: https://hudi.apache.org/docs/quick-start-guide

As long as you have spark 3.3 installed, you should be able to reproduce/re-run this with no issues.
Just replace the org.apache.hudi:hudi-spark3.3-bundle_2.12:0.13.1 with org.apache.hudi:hudi-spark3.3-bundle_2.12:0.14.1.
And Vice-Versa.
Stacktrace
Traceback (most recent call last):
  File "/Users/johng.wilsonnegroni/PycharmProjects/hudi_testing/venv/lib/python3.10/site-packages/pyspark/sql/utils.py", line 190, in deco
    return f(*a, **kw)
  File "/Users/johng.wilsonnegroni/PycharmProjects/hudi_testing/venv/lib/python3.10/site-packages/py4j/protocol.py", line 326, in get_return_value
    raise Py4JJavaError(
py4j.protocol.Py4JJavaError: An error occurred while calling o105.showString.
: org.apache.spark.SparkException: Job aborted due to stage failure: Task 0 in stage 78.0 failed 1 times, most recent failure: Lost task 0.0 in stage 78.0 (TID 110) (<IP_ADDRESS> executor driver): java.lang.NoClassDefFoundError: org/apache/hudi/com/fasterxml/jackson/module/scala/DefaultScalaModule$
	at org.apache.hudi.cdc.HoodieCDCRDD$CDCFileGroupIterator.<init>(HoodieCDCRDD.scala:237)
	at org.apache.hudi.cdc.HoodieCDCRDD.compute(HoodieCDCRDD.scala:101)
	at org.apache.spark.rdd.RDD.computeOrReadCheckpoint(RDD.scala:365)
	at org.apache.spark.rdd.RDD.iterator(RDD.scala:329)
	at org.apache.spark.rdd.MapPartitionsRDD.compute(MapPartitionsRDD.scala:52)
	at org.apache.spark.rdd.RDD.computeOrReadCheckpoint(RDD.scala:365)
	at org.apache.spark.rdd.RDD.iterator(RDD.scala:329)
	at org.apache.spark.rdd.MapPartitionsRDD.compute(MapPartitionsRDD.scala:52)
	at org.apache.spark.rdd.RDD.computeOrReadCheckpoint(RDD.scala:365)
	at org.apache.spark.rdd.RDD.iterator(RDD.scala:329)
	at org.apache.spark.scheduler.ResultTask.runTask(ResultTask.scala:90)
	at org.apache.spark.scheduler.Task.run(Task.scala:136)
	at org.apache.spark.executor.Executor$TaskRunner.$anonfun$run$3(Executor.scala:548)
	at org.apache.spark.util.Utils$.tryWithSafeFinally(Utils.scala:1504)
	at org.apache.spark.executor.Executor$TaskRunner.run(Executor.scala:551)
	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
	at java.lang.Thread.run(Thread.java:750)
Caused by: java.lang.ClassNotFoundException: org.apache.hudi.com.fasterxml.jackson.module.scala.DefaultScalaModule$
	at java.net.URLClassLoader.findClass(URLClassLoader.java:387)
	at java.lang.ClassLoader.loadClass(ClassLoader.java:418)
	at java.lang.ClassLoader.loadClass(ClassLoader.java:351)
	... 18 more

Driver stacktrace:
	at org.apache.spark.scheduler.DAGScheduler.failJobAndIndependentStages(DAGScheduler.scala:2672)
	at org.apache.spark.scheduler.DAGScheduler.$anonfun$abortStage$2(DAGScheduler.scala:2608)
	at org.apache.spark.scheduler.DAGScheduler.$anonfun$abortStage$2$adapted(DAGScheduler.scala:2607)
	at scala.collection.mutable.ResizableArray.foreach(ResizableArray.scala:62)
	at scala.collection.mutable.ResizableArray.foreach$(ResizableArray.scala:55)
	at scala.collection.mutable.ArrayBuffer.foreach(ArrayBuffer.scala:49)
	at org.apache.spark.scheduler.DAGScheduler.abortStage(DAGScheduler.scala:2607)
	at org.apache.spark.scheduler.DAGScheduler.$anonfun$handleTaskSetFailed$1(DAGScheduler.scala:1182)
	at org.apache.spark.scheduler.DAGScheduler.$anonfun$handleTaskSetFailed$1$adapted(DAGScheduler.scala:1182)
	at scala.Option.foreach(Option.scala:407)
	at org.apache.spark.scheduler.DAGScheduler.handleTaskSetFailed(DAGScheduler.scala:1182)
	at org.apache.spark.scheduler.DAGSchedulerEventProcessLoop.doOnReceive(DAGScheduler.scala:2860)
	at org.apache.spark.scheduler.DAGSchedulerEventProcessLoop.onReceive(DAGScheduler.scala:2802)
	at org.apache.spark.scheduler.DAGSchedulerEventProcessLoop.onReceive(DAGScheduler.scala:2791)
	at org.apache.spark.util.EventLoop$$anon$1.run(EventLoop.scala:49)
	at org.apache.spark.scheduler.DAGScheduler.runJob(DAGScheduler.scala:952)
	at org.apache.spark.SparkContext.runJob(SparkContext.scala:2228)
	at org.apache.spark.SparkContext.runJob(SparkContext.scala:2249)
	at org.apache.spark.SparkContext.runJob(SparkContext.scala:2268)
	at org.apache.spark.sql.execution.SparkPlan.executeTake(SparkPlan.scala:506)
	at org.apache.spark.sql.execution.SparkPlan.executeTake(SparkPlan.scala:459)
	at org.apache.spark.sql.execution.CollectLimitExec.executeCollect(limit.scala:48)
	at org.apache.spark.sql.Dataset.collectFromPlan(Dataset.scala:3868)
	at org.apache.spark.sql.Dataset.$anonfun$head$1(Dataset.scala:2863)
	at org.apache.spark.sql.Dataset.$anonfun$withAction$2(Dataset.scala:3858)
	at org.apache.spark.sql.execution.QueryExecution$.withInternalError(QueryExecution.scala:510)
	at org.apache.spark.sql.Dataset.$anonfun$withAction$1(Dataset.scala:3856)
	at org.apache.spark.sql.execution.SQLExecution$.$anonfun$withNewExecutionId$6(SQLExecution.scala:109)
	at org.apache.spark.sql.execution.SQLExecution$.withSQLConfPropagated(SQLExecution.scala:169)
	at org.apache.spark.sql.execution.SQLExecution$.$anonfun$withNewExecutionId$1(SQLExecution.scala:95)
	at org.apache.spark.sql.SparkSession.withActive(SparkSession.scala:779)
	at org.apache.spark.sql.execution.SQLExecution$.withNewExecutionId(SQLExecution.scala:64)
	at org.apache.spark.sql.Dataset.withAction(Dataset.scala:3856)
	at org.apache.spark.sql.Dataset.head(Dataset.scala:2863)
	at org.apache.spark.sql.Dataset.take(Dataset.scala:3084)
	at org.apache.spark.sql.Dataset.getRows(Dataset.scala:288)
	at org.apache.spark.sql.Dataset.showString(Dataset.scala:327)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:498)
	at py4j.reflection.MethodInvoker.invoke(MethodInvoker.java:244)
	at py4j.reflection.ReflectionEngine.invoke(ReflectionEngine.java:357)
	at py4j.Gateway.invoke(Gateway.java:282)
	at py4j.commands.AbstractCommand.invokeMethod(AbstractCommand.java:132)
	at py4j.commands.CallCommand.execute(CallCommand.java:79)
	at py4j.ClientServerConnection.waitForCommands(ClientServerConnection.java:182)
	at py4j.ClientServerConnection.run(ClientServerConnection.java:106)
	at java.lang.Thread.run(Thread.java:750)
Caused by: java.lang.NoClassDefFoundError: org/apache/hudi/com/fasterxml/jackson/module/scala/DefaultScalaModule$
	at org.apache.hudi.cdc.HoodieCDCRDD$CDCFileGroupIterator.<init>(HoodieCDCRDD.scala:237)
	at org.apache.hudi.cdc.HoodieCDCRDD.compute(HoodieCDCRDD.scala:101)
	at org.apache.spark.rdd.RDD.computeOrReadCheckpoint(RDD.scala:365)
	at org.apache.spark.rdd.RDD.iterator(RDD.scala:329)
	at org.apache.spark.rdd.MapPartitionsRDD.compute(MapPartitionsRDD.scala:52)
	at org.apache.spark.rdd.RDD.computeOrReadCheckpoint(RDD.scala:365)
	at org.apache.spark.rdd.RDD.iterator(RDD.scala:329)
	at org.apache.spark.rdd.MapPartitionsRDD.compute(MapPartitionsRDD.scala:52)
	at org.apache.spark.rdd.RDD.computeOrReadCheckpoint(RDD.scala:365)
	at org.apache.spark.rdd.RDD.iterator(RDD.scala:329)
	at org.apache.spark.scheduler.ResultTask.runTask(ResultTask.scala:90)
	at org.apache.spark.scheduler.Task.run(Task.scala:136)
	at org.apache.spark.executor.Executor$TaskRunner.$anonfun$run$3(Executor.scala:548)
	at org.apache.spark.util.Utils$.tryWithSafeFinally(Utils.scala:1504)
	at org.apache.spark.executor.Executor$TaskRunner.run(Executor.scala:551)
	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
	... 1 more
Caused by: java.lang.ClassNotFoundException: org.apache.hudi.com.fasterxml.jackson.module.scala.DefaultScalaModule$
	at java.net.URLClassLoader.findClass(URLClassLoader.java:387)
	at java.lang.ClassLoader.loadClass(ClassLoader.java:418)
	at java.lang.ClassLoader.loadClass(ClassLoader.java:351)
	... 18 more

Thanks for raising this @johnwilsonabartys . You are correct. With 0.14.1 version we are facing this issue while doing incremental CDC read. Interestingly with 0.14.0, we are not facing this issue.
Raised JIRA to track - https://issues.apache.org/jira/browse/HUDI-7360

The same happens with streaming source - since HoodieSourceOffset has
import com.fasterxml.jackson.module.scala.DefaultScalaModule.
As for 0.14.1 bundle - it has only
com.fasterxml.jackson.module.afterburner from jackson.

Is there a way to manually add the class after importing the spark bundle?

I also encountered the same problem. Under the error log information and the suggestion of that friend, I saw that the problem seemed to be that there was a configuration missing in pom.xml when packaging and compiling. I added the following line in the pom.xml file, and no problem occurred after testing.

I got it to compile, bootstrapped the spark bundle, hive sync, and aws bundle to emr.  Now getting java.lang.ClassNotFoundException: Class com.amazonaws.glue.catalog.metastore.AWSGlueDataCatalogHiveClientFactory not found

99% following these steps should fix issue for you
https://docs.aws.amazon.com/emr/latest/ReleaseGuide/emr-spark-glue.html
Namely
Specify the value for hive.metastore.client.factory.class using the
spark-hive-site classification as shown in the following example:
[
{
"Classification": "spark-hive-site",
"Properties": {
"hive.metastore.client.factory.class":
"com.amazonaws.glue.catalog.metastore.AWSGlueDataCatalogHiveClientFactory"
}
}
]
Because this class
com.amazonaws.glue.catalog.metastore.AWSGlueDataCatalogHiveClientFactory is
hidden inside AWS EMR(not available in any public repo - this is old
version of it but AWS no longer publish it
https://github.com/awslabs/aws-glue-data-catalog-client-for-apache-hive-metastore
).
On Fri, Mar 8, 2024 at 10:55 PM Tyler Rendina @.***>
wrote:

I got it to compile, bootstrapped the spark bundle, hive sync, and aws
bundle to emr. Now getting java.lang.ClassNotFoundException: Class
com.amazonaws.glue.catalog.metastore.AWSGlueDataCatalogHiveClientFactory
not found
—
Reply to this email directly, view it on GitHub
https://github.com/apache/hudi/issues/10590#issuecomment-1986474921, or
unsubscribe
https://github.com/notifications/unsubscribe-auth/ADZ45NKP4XHQVQRCKP7VKMLYXIXV7AVCNFSM6AAAAABCSD7ZLGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSOBWGQ3TIOJSGE
.
You are receiving this because you commented.Message ID:
@.***>

I have the properties set and it works with out of the box hudi versions.  I think it's got something to do with the custom hudi build I created.
Part of my bootstrap script
sudo mkdir -p /mnt1/hudi-jars
sudo chown -R $USER /mnt1/hudi-jars
sudo chmod -R ugo+rw /mnt1/hudi-jars
aws s3 cp s3://bucket/jars/hudi-aws-bundle-0.14.1.jar /mnt1/hudi-jars
aws s3 cp s3://bucket/jars/hudi-spark3.3-bundle_2.12-0.14.1.jar /mnt1/hudi-jars
aws s3 cp s3://bucket/jars/hudi-hive-sync-bundle-0.14.1.jar /mnt1/hudi-jars
sudo ln -sf /mnt1/hudi-jars/hudi-aws-bundle-0.14.1.jar hudi-aws-bundle.jar
sudo ln -sf /mnt1/hudi-jars/hudi-spark3.3-bundle_2.12-0.14.1.jar hudi-spark3.3-bundle.jar
sudo ln -sf /mnt1/hudi-jars/hudi-hive-sync-bundle-0.14.1.jar hudi-hive-sync-bundle.jar
PATH=$PATH:/mnt1/hudi-jars/

Part of my spark-submit
                        "--jars",
                        "/mnt1/hudi-jars/hudi-spark3.3-bundle_2.12-0.14.1.jar",
                        "/mnt1/hudi-jars/hudi-aws-bundle-0.14.1.jar,/mnt1/hudi-jars/hudi-spark3.3-bundle_2.12-0.14.1.jar,/mnt1/hudi-jars/hudi-hive-sync-bundle-0.14.1.jar",
                        "--conf",
                        "spark.driver.extraClassPath=/mnt1/hudi-jars/*",
                        "--conf",
                        "spark.executor.extraClassPath=/mnt1/hudi-jars/*",

                        "--packages",
                        "org.apache.hadoop:hadoop-aws:3.3.3,com.johnsnowlabs.nlp:spark-nlp_2.12:5.1.4",

I also encountered the same problem. Under the error log information and the suggestion of that friend, I saw that the problem seemed to be that there was a configuration missing in pom.xml when packaging and compiling. I added the following line in the pom.xml file, and no problem occurred after testing.

After building from 0.14.1 source with this line changed, I receive a scala version error
py4j.protocol.Py4JJavaError: An error occurred while calling None.org.apache.spark.api.java.JavaSparkContext.
: java.lang.ExceptionInInitializerError
	at org.apache.spark.scheduler.EventLoggingListener.initEventLog(EventLoggingListener.scala:89)
	at org.apache.spark.scheduler.EventLoggingListener.start(EventLoggingListener.scala:84)
	at org.apache.spark.SparkContext.<init>(SparkContext.scala:613)
	at org.apache.spark.api.java.JavaSparkContext.<init>(JavaSparkContext.scala:58)
	at sun.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)
	at sun.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:62)
	at sun.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45)
	at java.lang.reflect.Constructor.newInstance(Constructor.java:423)
	at py4j.reflection.MethodInvoker.invoke(MethodInvoker.java:247)
	at py4j.reflection.ReflectionEngine.invoke(ReflectionEngine.java:357)
	at py4j.Gateway.invoke(Gateway.java:238)
	at py4j.commands.ConstructorCommand.invokeConstructor(ConstructorCommand.java:80)
	at py4j.commands.ConstructorCommand.execute(ConstructorCommand.java:69)
	at py4j.ClientServerConnection.waitForCommands(ClientServerConnection.java:182)
	at py4j.ClientServerConnection.run(ClientServerConnection.java:106)
	at java.lang.Thread.run(Thread.java:750)
Caused by: com.fasterxml.jackson.databind.JsonMappingException: Scala module 2.13.4 requires Jackson Databind version >= 2.13.0 and < 2.14.0 - Found jackson-databind version 2.12.3
	at com.fasterxml.jackson.module.scala.JacksonModule.setupModule(JacksonModule.scala:61)
	at com.fasterxml.jackson.module.scala.JacksonModule.setupModule$(JacksonModule.scala:46)
	at com.fasterxml.jackson.module.scala.DefaultScalaModule.setupModule(DefaultScalaModule.scala:17)
	at com.fasterxml.jackson.databind.ObjectMapper.registerModule(ObjectMapper.java:835)
	at org.apache.spark.util.JsonProtocol$.<init>(JsonProtocol.scala:62)
	at org.apache.spark.util.JsonProtocol$.<clinit>(JsonProtocol.scala)
	... 16 more

Should I hard code the scala.binary.varsion as 2.12?

Final note, apologies for the amount of posts, but this may help EMR users with Glue as their Hive service.
Make sure to build Hudi using Java 8, if you are on ARM use something like Azul OpenJDK and export $JAVA_HOME as the provided path, i.e., /Library/Java/JavaVirtualMachines/zulu-8.jdk/Contents/Home.
Once you upload your jars to s3, bootstrap such as:
sudo chown -R $USER:root /usr/lib/hudi
sudo chmod -R ugo+rw /usr/lib/hudi
aws s3 cp s3://BUCKET/jars/hudi-aws-bundle-0.14.1.jar /usr/lib/hudi
aws s3 cp s3://BUCKET/jars/hudi-spark3.3-bundle_2.12-0.14.1.jar /usr/lib/hudi
sudo ln -sf /usr/lib/hudi/hudi-aws-bundle-0.14.1.jar hudi-aws-bundle.jar
sudo ln -sf /usr/lib/hudi/hudi-spark3.3-bundle_2.12-0.14.1.jar hudi-spark3.3-bundle.jar

To use your custom built Hudi package, conform to your bootstrap paths in the following spark submit command element:
"--jars",
"/usr/lib/hudi/hudi-aws-bundle-0.14.1.jar,/usr/lib/hudi/hudi-spark3.3-bundle_2.12-0.14.1.jar,",
"--conf",
"spark.driver.extraClassPath=/usr/lib/hudi/hudi-aws-bundle-0.14.1.jar:/usr/lib/hudi/hudi-spark3.3-bundle_2.12-0.14.1.jar",
"--conf",
"spark.executor.extraClassPath=/usr/lib/hudi/hudi-aws-bundle-0.14.1.jar:/usr/lib/hudi/hudi-spark3.3-bundle_2.12-0.14.1.jar",

Finally, (the issue I had with Class Not Found com.amazonaws.glue.catalog.metastore.AWSGlueDataCatalogHiveClientFactory), DO NOT use .enableHiveSupport() when setting your spark context, while this works with hudi imported with --packages, it will try to use the wrong hive package when you specify --jars.

While I can kick off backfills, they eventually fail along side streams with java.lang.NoSuchMethodError: com.amazonaws.transform.JsonUnmarshallerContext.getCurrentToken()Lcom/amazonaws/thirdparty/jackson/core/JsonToken;

This is fixed in
https://github.com/apache/hudi/pull/10877

why is it required to set these?
is it really required?
> "--conf",
> "spark.driver.extraClassPath=/usr/lib/hudi/hudi-aws-bundle-0.14.1.jar:/usr/lib/hudi/hudi-spark3.3-bundle_2.12-0.14.1.jar",
> "--conf",
> "spark.executor.extraClassPath=/usr/lib/hudi/hudi-aws-bundle-0.14.1.jar:/usr/lib/hudi/hudi-spark3.3-bundle_2.12-0.14.1.jar",

why is it required to set these? is it really required?
> "--conf",
> "spark.driver.extraClassPath=/usr/lib/hudi/hudi-aws-bundle-0.14.1.jar:/usr/lib/hudi/hudi-spark3.3-bundle_2.12-0.14.1.jar",
> "--conf",
> "spark.executor.extraClassPath=/usr/lib/hudi/hudi-aws-bundle-0.14.1.jar:/usr/lib/hudi/hudi-spark3.3-bundle_2.12-0.14.1.jar",

You can try without it. Basically /usr/lib/hudi/hudi-aws-bundle-0.14.1.jar includes minor things like AwsGlueCatalogSyncTool which behaves better then default if changed(https://hudi.apache.org/docs/configurations/#hoodiemetasyncclienttoolclass) on huge number of partitions.

why is it required to set these? is it really required?
> "--conf",
> "spark.driver.extraClassPath=/usr/lib/hudi/hudi-aws-bundle-0.14.1.jar:/usr/lib/hudi/hudi-spark3.3-bundle_2.12-0.14.1.jar",
> "--conf",
> "spark.executor.extraClassPath=/usr/lib/hudi/hudi-aws-bundle-0.14.1.jar:/usr/lib/hudi/hudi-spark3.3-bundle_2.12-0.14.1.jar",

You can try without it. Basically /usr/lib/hudi/hudi-aws-bundle-0.14.1.jar includes minor things like AwsGlueCatalogSyncTool which behaves better then default if changed(https://hudi.apache.org/docs/configurations/#hoodiemetasyncclienttoolclass) on huge number of partitions.

On the spark config side: Any dependencies off of the default spark path will require setting the extra class path in two places so both the driver and executor containers can see them.  You can use blob statements to make this more concise if there are no conflicts.
My comments on this thread are based around the EMR environment so the long class path after the fact is also required if using Glue metastore.

why is it required to set these? is it really required?
> "--conf",
> "spark.driver.extraClassPath=/usr/lib/hudi/hudi-aws-bundle-0.14.1.jar:/usr/lib/hudi/hudi-spark3.3-bundle_2.12-0.14.1.jar",
> "--conf",
> "spark.executor.extraClassPath=/usr/lib/hudi/hudi-aws-bundle-0.14.1.jar:/usr/lib/hudi/hudi-spark3.3-bundle_2.12-0.14.1.jar",

You can try without it. Basically /usr/lib/hudi/hudi-aws-bundle-0.14.1.jar includes minor things like AwsGlueCatalogSyncTool which behaves better then default if changed(https://hudi.apache.org/docs/configurations/#hoodiemetasyncclienttoolclass) on huge number of partitions.

On the spark config side: Any dependencies off of the default spark path will require setting the extra class path in two places so both the driver and executor containers can see them. You can use blob statements to make this more concise if there are no conflicts.
My comments on this thread are based around the EMR environment so the long class path after the fact is also required if using Glue metastore.

Thank you very much for your comment.
I am on Amazon EMR on EKS env as well.
I've been using --jar option only to load and use the Hudi bundles from either Maven or Amazon EMR.
( The JARs bundles either on S3 or in the Amazon EMR Image. )
What I am curious is --conf is required along with --jar?
Thank you

@VitoMakarevich
Thank you for your response.
I've been using --jar only to load Hudi bundles ( haven't modified the "spark.driver.extraClassPath" or "spark.executor.extraClassPath" when using other Hudi bundles which are different from the shipped one in the Amazon EMR.
That's why I am curios if setting "spark.driver.extraClassPath" or "spark.executor.extraClassPath" are required along with --jar
Thank you

EMR on EKS gave me issues and I switched to EMR on EC2 about a year ago, probably needed to do the same thing done here.  Planning to use something like kubeflow in the future.
When you spin up a job and look at the environment vars in the spark UI you can ctrl + f for 'classpath' and see what the key value options are.  If the location of the jar is not on the classpath then it must be specified in addition to the initial classpath specified in that var of the spark UI.
In this case I was using --packages to get them directly from Maven, then I tried --jar and ran into version issues, hence the custom build above.  This led to using the custom jars from s3.  I imagine writing a more robust bootstrap script may have mitigated the issue.  I also wanted to make sure the jar selected for hudi was not the preinstalled version in the particular case.

@Tyler-Rendina

EMR on EKS gave me issues and I switched to EMR on EC2 about a year ago, probably needed to do the same thing done here. Planning to use something like kubeflow in the future.
When you spin up a job and look at the environment vars in the spark UI you can ctrl + f for 'classpath' and see what the key value options are. If the location of the jar is not on the classpath then it must be specified in addition to the initial classpath specified in that var of the spark UI.
In this case I was using --packages to get them directly from Maven, then I tried --jar and ran into version issues, hence the custom build above. This led to using the custom jars from s3. I imagine writing a more robust bootstrap script may have mitigated the issue. I also wanted to make sure the jar selected for hudi was not the preinstalled version in the particular case.

Thank you very much for the detailed explanation.
Let me check the environment vars in the spark UI you can ctrl + f for 'classpath'


Improve tooltip behaviour/content for production release.

What needs to be done (copied from @devoncarew  in #817):

debounce, so that it shows when the mouse cursor is quiet for a short period
borrow style (and content) from the current F1 dialog. It doesn't need to look the same; I think there's room for improvement in that dialog
ensure that users are able to scroll within a tooltip w/ a lot of content w/o the tooltip disappearing when they move their mouse
remove the F1 dialog
remove the experimental flag

@devoncarew Are we sure we want it to replace the F1 dartdoc popup completely? It might be heavy to show all this information every time.

I think once we have this people won't use the F1 dialog anymore. The tooltip dialog should perhaps be a bit more concise or compact than the existing dartdoc dialog.
If we think this won't work we could have an 'expand' button on the tooltip dialog, but I'm not sure that will be necessary.


startDate booking today issue with timezone

While I'm testing on someone code as I notice the start date on the Airbnb/react-dates as I just found an issue. while I am booking the car for hire, as I want it now,
If I book at the morning for today, it should be no problem, if I book before 12 for today, it's ok, but when it comes after  12:00 to upward  I'm unable to book it for today. it supposed to be up to 00:01 to 23:59 hours, not 12:00 to force disabled to book today, if I book evening for today it should be working without any issue.
I suspend the problem is more backend relation as I have look this code and I think it correct as I cannot find the pinpoint
this.state = {
      startDate: this.props.startDate,
      endDate: this.props.endDate,
      pricingQuote: {},
      quotation: {},
      loadingRates: false,
      verificationsNeeded: false,
      numberOfMonthsToShow: Helpers.pageWidth() >= 768 ? 2 : 1,
      daySize: Helpers.pageWidth() < 400 ? Math.round((Helpers.pageWidth() - 90) / 7) : null,
      loading: false,
      showUserVerificationModal: false
    };

I am aware it's not enough information, could be a backend issue or internationalize time issue?

found this code as it was another pages, please disregard this


Not working when opening files directly without a workspace/folder

https://github.com/emeraldwalk/vscode-runonsave/blob/dae1b6624e8de698e94263d36a1b1bb905ca5e03/src/extension.ts#L189

I have the same problem.


Create TESTING.md Documentation

Description
As we build out Nachet, it's essential to have proper documentation that guides testers through manual testing procedures. This document will provide a standardized approach to testing new features and will ensure that all functionality is verified before deployment.
Scope

Create a TESTING.md file in the project root.
Document should outline the steps required to manually test the application's functionality.
Include sample test cases that cover various scenarios and edge cases.

Requirements

The TESTING.md should be clear and concise.
Test steps should be numbered for clarity.
Include preconditions or setup steps necessary for testing.
Describe expected outcomes and how to interpret the test results.
Provide commands or actions in a copy-pastable format when relevant.
Include screenshots or GIFs for UI-related tests if possible.

Sample Test Case Template
## Test Case Title

**Preconditions:**
- [ ] List any setup required before testing.

**Test Steps:**
1. Step one to perform (e.g., Click on the 'Results' tab).
2. Step two to perform (e.g., Select 'Model 2' from the dropdown menu).
3. Continue with additional steps...

**Expected Results:**
- [ ] Describe what the tester should expect to see.

**Actual Results:**
- [ ] Tester fills out after executing the test.

**Pass/Fail:**
- [ ] Tester marks after performing the test.

I like your TESTING.md template. and your description of what is required for manual testing. It would be worth it to move this into dev-rel-docs and get the team to review and adopt this (and link from this issue to that new doc in dev-rel-docs).


operator mariadb-operator (0.6.6)

Add operator mariadb-operator (0.6.6) to community operators

/merge possible

/merge possible


Animate annotations when chart has live data

Is your feature request related to a problem? Please describe.
If you have a line chart with realtime data and you want to add vertical annotations, they will move periodically based on "dynamicAnimation.speed" property.
In practice this means for example that line chart series is being drawn in real time but annotation jumps to correct position only once per second.
Describe the solution you'd like
So in perfect world the correct solution would be that annotation gets moved with the same speed as line chart series is being drawn, resulting in very "live" animation.
Describe alternatives you've considered
There isn't any "non hackish" solution for this problem unless I would manually draw annotations to graph canvas.

I would like this too. Anyone a solution for this?

This issue isn't fixed yet, why is it closed?


Add ADR about time graph filtering TSP updates

Signed-off-by: hriday-panchasara<EMAIL_ADDRESS>

Added a sequence diagram to show the order of http operations to make the filter work. This diagram is pretty basic, I was not sure if I needed to go in depth.


fix maxFillOpacity

Fixes maxFillOpacity not being respected. Repro of the issue:
import LajiMap from "../src/index.ts";
import * as testData from "./testData.js";
import "../src/styles.ts";

const data = {
	featureCollection: testData.testData.featureCollection,
	getFeatureStyle: () => ({
		stroke: false, fillColor: "#ff0000", fillOpacity: 1
	}),
	polygon: { maxFillOpacity: 1 }
};

new LajiMap({
	rootElem: document.getElementById("root"),
	tileLayerName: "taustakartta",
	data: [data]
});

The issue was that _getStyleForType references item.group, which has not been initialized yet. Moved the definition of the style function to ensure that item.group is initialized first.
(note: I haven't run the tests)

I'm a bit hesitant about this change, because it might affect performance of data initialization quite a bit, since it needs to set the style for layer again. Using L.geoJSONs style param doesn't have a performance penalty.
It's quite a mess right now... the _getStyleForType needs some refactoring. It's so complicated because sometimes the layer is undefined, and sometimes feature is undefined. We can get rid of the layer param if we drop support for customizing rectangle options, and just use polygon options instead (looking at a feature it's impossible to say whether it's a "rectangle", since that's not a GeoJSON thing but Leaflet's own implementation). I'll look into this

Fixed by https://github.com/luomus/laji-map/commit/668af2a1bf9d216dcdd1d07ac83e20c5418d45b6


Docker Compose: PermissionError: [Errno 13] Permission denied: '/var/lib/langflow/secret_key'

On a fresh Ubuntu server, I follow the instructions on the Langflow Docker deployment documentation. When running docker compose up, I get the following error in the langflow service:

PermissionError: [Errno 13] Permission denied: '/var/lib/langflow/secret_key'

To Reproduce
Steps to reproduce the behavior:

Create an Ubuntu VPS instance
Git clone the repository
cd to the docker_example directory
run docker compose up
See error

Got the same error here, I've with few older version of the docker image, same result each time.

I've worked around the permissions issue by using a Dockerfile to create and change the ownership of /var/lib/langflow, but that only lead to another startup issue (see Langflow discord https://discord.com/channels/1116803230643527710/1256237409692029013/1256237409692029013).
I see the docker-compose.yml has an updated config path /app/langflow now, but that leads to the same permissions issue.

I've opened a pull request as a proof-of-concept. I'm on my mobile and coding with GitHub Copilot so can't currently test the PR. Also, I'm not sure whether the PR is the ideal solution, so opened it mainly to continue the discussion 🙂

Hi @brylie  ,
Do you need any assistance with this case? If not, please let us know if this issue can be closed.


CI: ensure all git checkout steps use same id

the current pipeline has "unsynchronized" checkout statements, i.e. in a rare case we could end up building a different repo state for conda and pypi.
Task: record the git hash in the prepare step and pass it to the build steps.

#74 includes a switch to stash/unstash, i.e. we checkout once and then re-use that state


Use latest versions of docker and CentOs

Relevant kubevirtci PR - https://github.com/kubevirt/kubevirtci/pull/13
Signed-off-by: Lukianov Artyom<EMAIL_ADDRESS>

retest this please

retest this please

retest this please

I will re-run tests again before we merge it

hh too late :smile:


Add flake8 Python linting

This PR adds a flake8 config file, adds flake8 to travis, and cleans up the Python style everything except for migrations and cfgov/v1/tests
There are a few things I want to bring up for discussion based on this:

By ignoring v1/tests, this configuration will let us run flake8 in Travis and ensure that the cleanup in this PR does not regress. However, it has a side-effect, in that anytime one runs flake8 from the root of cfgov-refresh, it will ignore v1/tests.

There is some pretty serious refactoring needed that this PR does not do, and which I is out of scope. This if condition is probably the worst example, but there are other less bad examples as well.

This still doesn't help urls.py very much

I am completely ignoring our settings files. It strikes me that the readability of those might have different concerns than simply Python linting, but I'd welcome some discussion of that.

I'd love to include a max-complexity test, but currently there are parts of the cleaned code that fail (like this. This goes back to the need for refactoring, above.

So, I'm tagging all the back-ends that are working on cfgov-refresh right now (from platform and data and research) so that we can hash out some of these.
Once we work through this, I'd like to squash these commits when we merge, so there can be only one.
Additions

flake8 config file
flake8 dependencies
flake8 as a default travis command

Changes

General flake8-compliant style cleanup

Testing

Run flake8 in the root of cfgov-refresh. There should be no errors.

Review

@chosak
@richaagarwal
@grapesmoker
@higs4281
@rosskarchner
@hillaryj

Checklist

[X] Changes are limited to a single goal (no scope creep)
[X] Code can be automatically merged (no conflicts)
[ ] Code follows the standards laid out in the front end playbook
[X] Passes all existing automated tests
[ ] New functions include new tests
[ ] New functions are documented (with a description, list of inputs, and expected output)
[ ] Placeholder code is flagged
[ ] Visually tested in supported browsers and devices
[ ] Project documentation has been updated (including the "Unreleased" section of the CHANGELOG)

I meant to add reasons for why each of those ignored packages is ignored:

v1: too much potential for conflict. We'll need to handle this one piecemeal.
jobmanager: most of the flake8 complaints are with the Django models and admin, which we'll be cleaning up anyway.
data_research: I'm not sure if there's a potential for me to introduce conflicts with work-in-progress here. @hillaryj?

@willbarton data_research doesn't have any outstanding changes, so flake8 shouldn't introduce conflicts.

@hillaryj yay! I'll take care of it before it does:D

Update: apparently flake8's --exclude overrides the configured value, so it's impossible to configure all the other paths and then include --exclude v1 in the invocation. You have to specify all the exclusions either on the command line or in the config file. So, splitting them up is not really an option.

count me in team (...)
question for @chosak:
datetime is part of the standard library, so why separate it from the re import?

@higs4281 I like the Django style guide for imports, which is slightly stricter (emphasis added):

Put imports in these groups: future, standard library, third-party libraries, other Django components, local Django component, try/excepts. Sort lines in each group alphabetically by the full module name. Place all import module statements before from module import objects in each section.

@chosak sold!

Bringing this PR back up to date, @chosak and @higs4281. I've addressed the imports as they exist now (but I'd like to find a solution to checking them with flake8), and I've added the jobmanager. The only Python code in cfogv/ that I haven't cleaned up yet is v1.
I'm inclined to get this in, if it looks good to you all, and then we can address a cleanup of v1. This will give us automated linting of all the Python that's not in v1 already.

👍 monumental!

I was initially worried about leaving cfgov/core without any models.py file, because it has management commands. But that was a pre-1.7 problem, and the commands run without error on this branch, as do migration operations. So forward ho!

Do you/ will you need help resolving all the conflicts?  Also, what would it take to get this to a ready state so that there isn't an ongoing conflict mess?

@richaagarwal nah, I'll take care of the conflicts. I'd like to see that we're okay merging it before I do though, because otherwise I'll just have to do it again :)
FWIW, I've been merging master in with -s theirs so that style choices in this PR don't override code changes that've been made elsewhere.

@richaagarwal @chosak @higs4281 @rosskarchner Alright, This is once again up-to-date post-machete. What can we do to get this merged and have some automatic linting of Python PRs?


fixit: create new Java sample for aiplatform_create_custom_job_sample

Description
Fixes #281693980
Added missing Java sample for aiplatform_create_custom_job_sample
Note: Before submitting a pull request, please open an issue for discussion if you are not associated with Google.
Checklist

[ ] I have followed Sample Format Guide
[ ] pom.xml parent set to latest shared-configuration
[ ] Appropriate changes to README are included in PR
[ ] These samples need a new API enabled in testing projects to pass (let us know which ones)
[ ] These samples need a new/updated env vars in testing projects set to pass (let us know which ones)
[ ] Tests pass:   mvn clean verify required
[ ] Lint  passes: mvn -P lint checkstyle:check required
[ ] Static Analysis:  mvn -P lint clean compile pmd:cpd-check spotbugs:check advisory only
[ ] This sample adds a new sample directory, and I updated the CODEOWNERS file with the codeowners for this sample
[ ] This sample adds a new Product API, and I updated the Blunderbuss issue/PR auto-assigner with the codeowners for this sample
[ ] Please merge this PR for me once it is approved

/gcbrun

/gcbrun


[Snyk] Upgrade @stdlib/math-base-special-ldexp from 0.0.5 to 0.0.8

This PR was automatically created by Snyk using the credentials of a real user.Snyk has created this PR to upgrade @stdlib/math-base-special-ldexp from 0.0.5 to 0.0.8.
:information_source: Keep your dependencies up-to-date. This makes it easier to fix existing vulnerabilities and to more quickly identify and fix newly disclosed vulnerabilities when they affect your project.

The recommended version is 3 versions ahead of your current version.
The recommended version was released 5 months ago, on 2022-11-15.

Note: You are seeing this because you or someone else with access to this repository has authorized Snyk to open upgrade PRs.
For more information:
🧐 View latest project report
🛠 Adjust upgrade PR settings
🔕 Ignore this dependency or unsubscribe from future upgrade PRs

@zikaari I have some critical vulns from minimist coming from @segment/analytics-next, but this package is sort of the root of what you all own at segment. Would you mind giving this PR some love at your earliest convenience? Happy to help in any way I can.

I filed https://github.com/segmentio/tsub-js/issues/45 related to this as well -> without this, you can't install without an npm override (on Mac at least). A number of tickets have been filed on this -> can we get an update in on this one?

@saghaulor I asked on a related issue, but any path to getting this in? This is still blocking installing segment's analytics-nxt JS package at all for us (and others): https://github.com/segmentio/tsub-js/issues/45 and https://github.com/segmentio/analytics-next/issues/1044

The latest version for this dependency is 0.2.2, therefore please update this PR to upgrade it to latest version or create a new one.


Exception in thread "main" java.lang.UnsatisfiedLinkError: org.sqlite.core.NativeDB._open(Ljava/lang/String;I)V

Exception in thread "main" java.lang.UnsatisfiedLinkError: org.sqlite.core.NativeDB._open(Ljava/lang/String;I)V         at org.sqlite.core.NativeDB._open(Native Method)         at org.sqlite.core.DB.open(DB.java:161)         at org.sqlite.core.CoreConnection.open(CoreConnection.java:220)         at org.sqlite.core.CoreConnection.<init>(CoreConnection.java:76)         at org.sqlite.jdbc3.JDBC3Connection.<init>(JDBC3Connection.java:24)         at org.sqlite.jdbc4.JDBC4Connection.<init>(JDBC4Connection.java:23)         at org.sqlite.SQLiteConnection.<init>(SQLiteConnection.java:45)         at org.sqlite.JDBC.createConnection(JDBC.java:114)         at org.sqlite.JDBC.connect(JDBC.java:88)         at java.sql.DriverManager.getConnection(DriverManager.java:664)         at java.sql.DriverManager.getConnection(DriverManager.java:270)         at SQLiteJDBC.main(SQLiteJDBC.java:11)
I use  a raspberry pi 3, OS  raspbian (based on Debian 8 jessie), jdk and java VM below

Also tested openjdk8, and failed.Cry
I use sqlite-jdbc-<IP_ADDRESS>.jar.I tested this jar on my Mac(use jdk 1.7) and Windows(also use jdk 1.7), it worked fine.
my test code below
import java.sql.*;
public class SQLiteJDBC
{
public static void main( String args[] )
{
Connection con = null;
try {
Class.forName("org.sqlite.JDBC");
con = DriverManager.getConnection("jdbc:sqlite:/home/pi/Documents/arp.db");
} catch ( Exception e ) {
System.err.println( e.getClass().getName() + ": " + e.getMessage() );
System.exit(0);
}
System.out.println("Opened database successfully");
}
}
What can I do next?

I'm having the same problem, but I'm using a Raspberry Pi vB+.

I'm having the same problem...Also used  Raspbian PI 3

on Pi3 model B, it works with <IP_ADDRESS>.. for me, at least

@ideniden Have a look at https://github.com/xerial/sqlite-jdbc/issues/58 . You need to use a patched version of the library. Download here: https://mega.nz/#!RVlylBQL!w6ifnhXdtv6gqMtxLwEXBQjGJqQCsHIRnf1oZsafjdw

@kwantumWare Thanks very mach,Problem has been solved.^-^

Could you try the binary I built here?
https://oss.sonatype.org/content/repositories/snapshots/org/xerial/sqlite-jdbc/3.14.2-SNAPSHOT/
I'm using docker for building Raspberry Pi binary #151

Let me merge this issue to #58


Send out more invites for market research survey

We were close to the sample size we targeted (Confidence level: 90%, Margin of error: 10% = 68). But for some of the questions, we didn't see enough responses.
Done when

[x] 3100 more invites are sent with the new deadline set to Dec 6 2024 in both the French and English invite.

Done and we received 242 more totalling to 320 answers !


updated Communication link to point at our channel

please make sure - https://github.com/IBM/FHIR/pull/327 agrees with this PR

closed in favor of #327


goog.asserts.assert()

hello everyone,
can anyone please tell me how goog.asserts.assert() work???

Hi @anshgo01yal, goog.asserts.assert checks whether a condition (provided as the first argument) evaluates to true, and throws if it does -- although this depends on whether ENABLE_ASSERTS define is set to true. This is from the inline docs: https://github.com/google/closure-library/blob/0b41955ad8d55aebd2217de647a6de3a8c63916e/closure/goog/asserts/asserts.js#L169-L187
Is that what you were looking for, or did you have a more specific question?

thanks @kjin this exactly what I am asking for.
do you have any documentation link where I can learn about goog.assert , goog.provide  and all such functions

@anshgo01yal you might find the API reference for goog.asserts helpful in this context (and others).

thanks @12wrigja.


You should continue with development

Firstly, thank you on working on such needed library. Regardless of how many stars (*) you get, you continue working on the and make it better. As times goes people will see value.
..Ben

@BenHayat Thank you so much for your inspiring words. Surely I shall continue to develop this library. Please submit issues if you find or feature request if needed. I shall fix or add these as soon as possible.

@BenHayat Thank you so much for your inspiring words. Surely I shall continue to develop this library. Please submit issues if you find or feature request if needed. I shall fix or add these as soon as possible.

What I wrote to you, is something I say to myself everyday for my app. "Build it first, then people will come". If you build something that people need (and a solid repository is one), then it will expand. Just do it to fulfill your desire.
I'll be looking into docs now and give it some serious look & test and keep you posted.
Thanks for creating it.

I also suggest to offer a sample app that can guide people on setting up Entities and related files and folders.
This way,it helps where everyone uses a prescribed pattern.


Dispose Tensors as needed during async stream processing.

This approach differs from the prior plan to introduce synchronous streams.  We can still discuss different designs, but this PR at least makes the current design work for now.
The idea is that streams remain fully async, pull-only, source streams.  Because next() is async, it is not possible to include it within a dl.tidy().  At the same time, Tensors returned from next() must be protected from being inadvertently tidied.  Thus, all Tensors returned from a stream have already had dl.keep() called on them, and so must be manually disposed.
This PR ensures that all Tensors produced as intermediate results during pipeline operation are automatically disposed, so that the only ones that the user is responsible for are those returned from next().
Note this disallows passing async user functions to map(), filter(), and forEach().  Those user functions will be automatically wrapped in dl.tidy().

This change is

Happy to chat more about this in person, would be good to understand next steps for the memory management stuff.

Review status: 0 of 7 files reviewed at latest revision, 16 unresolved discussions, some commit checks failed.

src/contrib/data/batch_dataset.ts, line 19 at r1 (raw file):

 */

import * as dl from '../../index';

this will introduce a cycle, import keep directly from tracking.ts

src/contrib/data/batch_dataset_test.ts, line 18 at r1 (raw file):

 */

import {DatasetBatch} from '..';

import from the file directly

src/contrib/data/batch_dataset_test.ts, line 95 at r1 (raw file):

});

function consume(input: DatasetBatch): void {

can we put this in some common util somewhere, we seem to use it in several places?
And can we maybe just call this disposeBatch or something to that effect?

src/contrib/data/dataset.ts, line 287 at r1 (raw file):

        f(input);
        consume(input);
        return {};

why return {} at all?

src/contrib/data/dataset.ts, line 360 at r1 (raw file):

 * Tensors are still present in the output.
 */
function consumePrep(input: DatasetElement): (output: DatasetElement) => void {

move this logic out to a util, this code looks quite similar to the code in batch_dataset

src/contrib/data/dataset.ts, line 384 at r1 (raw file):

}

function retain(input: DatasetElement): DatasetElement {

can we call this something like keepDatasetElement so it's the same terminology as keep()?

src/contrib/data/dataset_test.ts, line 19 at r1 (raw file):

 */

import {keep} from '../../globals';

you can use keep via dl here (since tests use the public API).

src/contrib/data/dataset_test.ts, line 63 at r1 (raw file):

}

describeWithFlags('Dataset', ALL_ENVS, () => {

use CPU_ENVs, no need to run this everywhere.

src/contrib/data/dataset_test.ts, line 213 at r1 (raw file):

        return {};
      })
        .then(() => expect(count).toEqual(100))

use await in your tests for readability.

src/contrib/data/streams/data_stream.ts, line 150 at r1 (raw file):

   * @returns A `DataStream` of transformed elements.
   */
  map<OUT>(

is OUT a generic? If so, use "O".

src/contrib/data/streams/data_stream.ts, line 152 at r1 (raw file):

  map<OUT>(
      transform: (value: T) => OUT,
      consumePrep?: (item: T) => ((output: OUT) => void),

add jsdocs, "consumePrep" on it's own doesn't tell me exactly what it does

src/contrib/data/streams/data_stream.ts, line 288 at r1 (raw file):

    while (this.count++ < this.maxCount) {
      const skipped = await this.upstream.next();
      if (this.consume != null) this.consume(skipped);

use {}

src/contrib/data/streams/data_stream.ts, line 402 at r1 (raw file):

      protected upstream: DataStream<T>,
      protected predicate: (value: T) => boolean,
      protected consume: (item: T) => void) {

this is optional, right?

src/contrib/data/streams/data_stream.ts, line 413 at r1 (raw file):

    if (this.predicate(item)) {
      this.outputQueue.push(item);
    } else if (this.consume != null)

use {}

src/contrib/data/streams/data_stream.ts, line 419 at r1 (raw file):

}

class MapStream<IN, OUT> extends QueueStream<OUT> {

can you make these I and O for consistency with generics, and everywhere else in this PR?

src/contrib/data/streams/data_stream.ts, line 438 at r1 (raw file):

    let mapped = this.transform(item);

    if (this.retain != null) mapped = this.retain(mapped);

use {} here and below (ts style)

Comments from Reviewable

Maybe that convo can be our sync tomorrow when Daniel gets back

Review status: 0 of 7 files reviewed at latest revision, 16 unresolved discussions, some commit checks failed.

Comments from Reviewable

You bet, happy to chat.  Thanks for the review!

Review status: 0 of 7 files reviewed at latest revision, 17 unresolved discussions, some commit checks failed.

src/contrib/data/batch_dataset.ts, line 19 at r1 (raw file):
Previously, nsthorat (Nikhil Thorat) wrote…
this will introduce a cycle, import keep directly from tracking.ts

Oops quite so, thanks.  I fixed a pile of these already but apparently missed a couple.

src/contrib/data/batch_dataset_test.ts, line 18 at r1 (raw file):
Previously, nsthorat (Nikhil Thorat) wrote…
import from the file directly

Done.

src/contrib/data/batch_dataset_test.ts, line 95 at r1 (raw file):
Previously, nsthorat (Nikhil Thorat) wrote…
can we put this in some common util somewhere, we seem to use it in several places?
And can we maybe just call this disposeBatch or something to that effect?

I did rename all these.  Re putting them in a common util class: they're all private.  Do you think the util consolidation is worth the cost of unnecessarily exporting them?  They are actually all a bit different, too-- sure a few lines here and there might be factored out, but not all that much.

src/contrib/data/dataset.ts, line 287 at r1 (raw file):
Previously, nsthorat (Nikhil Thorat) wrote…
why return {} at all?

That's because the current implementation interprets null or undefined as "end of stream".  I'm already fixing this in the next PR by switching next() to return IteratorResult<T> aka {value:T, done:boolean} throughout.

src/contrib/data/dataset.ts, line 360 at r1 (raw file):
Previously, nsthorat (Nikhil Thorat) wrote…
move this logic out to a util, this code looks quite similar to the code in batch_dataset

See above: similar but not the same, and private.  Happy to move it out if you still prefer it that way.  Another approach would be to graduate DatasetElement and DatasetBatch to full classes, which could then have keep(), dispose(), and disposePrep() methods.

src/contrib/data/dataset.ts, line 384 at r1 (raw file):
Previously, nsthorat (Nikhil Thorat) wrote…
can we call this something like keepDatasetElement so it's the same terminology as keep()?

Done.

src/contrib/data/dataset_test.ts, line 19 at r1 (raw file):
Previously, nsthorat (Nikhil Thorat) wrote…
you can use keep via dl here (since tests use the public API).

Did that one for now.
There are tons of other direct imports though, in this code and in DLJS core tests.  I get that non-test code must use direct imports, but is there a reason to prefer one or the other style in tests?  Is consistency a good enough reason to pick one?

src/contrib/data/dataset_test.ts, line 63 at r1 (raw file):
Previously, nsthorat (Nikhil Thorat) wrote…
use CPU_ENVs, no need to run this everywhere.

Did this one.  I left batch_dataset_test and statistics_test alone for now, on the theory that they do Tensor manipulations that should be tested on GPU.

src/contrib/data/dataset_test.ts, line 213 at r1 (raw file):
Previously, nsthorat (Nikhil Thorat) wrote…
use await in your tests for readability.

Oh wow, for some reason I thought it() wouldn't accept an async function, but apparently it does.  Excellent-- these tests are fixed and I'll update the rest in a followup PR.

src/contrib/data/streams/data_stream.ts, line 150 at r1 (raw file):
Previously, nsthorat (Nikhil Thorat) wrote…
is OUT a generic? If so, use "O".

Done, though I'm curious why.  OUT is a perfectly valid generic type variable, and seems much clearer to me here.

src/contrib/data/streams/data_stream.ts, line 152 at r1 (raw file):
Previously, nsthorat (Nikhil Thorat) wrote…
add jsdocs, "consumePrep" on it's own doesn't tell me exactly what it does

Oh jeez, I meant to do that and forgot, thanks.  Any interest in adding a tslint "completed-docs" rule to catch these in the future?  Low priority of course.

src/contrib/data/streams/data_stream.ts, line 288 at r1 (raw file):
Previously, nsthorat (Nikhil Thorat) wrote…
use {}

Done.

src/contrib/data/streams/data_stream.ts, line 402 at r1 (raw file):
Previously, nsthorat (Nikhil Thorat) wrote…
this is optional, right?

Yep good catch!

src/contrib/data/streams/data_stream.ts, line 413 at r1 (raw file):
Previously, nsthorat (Nikhil Thorat) wrote…
use {}

Done.

src/contrib/data/streams/data_stream.ts, line 419 at r1 (raw file):
Previously, nsthorat (Nikhil Thorat) wrote…
can you make these I and O for consistency with generics, and everywhere else in this PR?

Done

src/contrib/data/streams/data_stream.ts, line 438 at r1 (raw file):
Previously, nsthorat (Nikhil Thorat) wrote…
use {} here and below (ts style)

Done.

Comments from Reviewable

Review status: 0 of 7 files reviewed at latest revision, 16 unresolved discussions, some commit checks failed.

src/contrib/data/dataset.ts, line 287 at r1 (raw file):
Previously, davidsoergel (David Soergel) wrote…
That's because the current implementation interprets null or undefined as "end of stream".  I'm already fixing this in the next PR by switching next() to return IteratorResult<T> aka {value:T, done:boolean} throughout.

Oh, and I realized that the forEach implementation consumes its inputs by default anyway, given a consumePrep function, so this whole bit was unnecessary anyway.

Comments from Reviewable

Few more nits ;)

Review status: 0 of 7 files reviewed at latest revision, 14 unresolved discussions.

src/contrib/data/batch_dataset_test.ts, line 95 at r1 (raw file):
Previously, davidsoergel (David Soergel) wrote…
I did rename all these.  Re putting them in a common util class: they're all private.  Do you think the util consolidation is worth the cost of unnecessarily exporting them?  They are actually all a bit different, too-- sure a few lines here and there might be factored out, but not all that much.

I think it's still worth it to try to consolidate. File-level exports will never get exported at the module level unless explicitly told to do so, so I wouldn't worry about extra file-level exports.
Ways to consolidate:

for disposeBatchPrep, it doesn't matter that the function has an argument, you can simply use () => with no args since you don't use the argument (typescript will be happy). Then you can always taken an array of DatasetElements or a single DatasetElement and return that function.
Same for extractTensorsFromElementArray, it can take a single one or an array.

src/contrib/data/dataset.ts, line 287 at r1 (raw file):
Previously, davidsoergel (David Soergel) wrote…
Oh, and I realized that the forEach implementation consumes its inputs by default anyway, given a consumePrep function, so this whole bit was unnecessary anyway.

Awesome. One more small thing, can you s/undefined/null

src/contrib/data/dataset_test.ts, line 19 at r1 (raw file):
Previously, davidsoergel (David Soergel) wrote…
Did that one for now.
There are tons of other direct imports though, in this code and in DLJS core tests.  I get that non-test code must use direct imports, but is there a reason to prefer one or the other style in tests?  Is consistency a good enough reason to pick one?

Yeah, we've been slowly migrating to this convention:

Tests import public API as dl through './index' unless they are testing something not module-exported
Internal library code does not import through index (because of cycles), it depends on the files that export our factory functions

src/contrib/data/dataset_test.ts, line 147 at r2 (raw file):

    const items = await ds.collectAll();
    expect(items.length).toEqual(100);
    expect(dl.memory().numTensors).toEqual(200);

why 200? where does the 2 come from?

src/contrib/data/dataset_test.ts, line 154 at r2 (raw file):

    expect(dl.memory().numTensors).toEqual(0);
    await ds.skip(15).collectAll();
    expect(dl.memory().numTensors).toEqual(170);

Why 170? Can you make it clear here either with a variable or a comment? If I just read this without understanding skip, I would assume this is testing that we have a leak :)
Other places in this file too that you assert a non-zero constant

src/contrib/data/streams/data_stream.ts, line 150 at r1 (raw file):
Previously, davidsoergel (David Soergel) wrote…
Done, though I'm curious why.  OUT is a perfectly valid generic type variable, and seems much clearer to me here.

Convention :)

src/contrib/data/streams/data_stream.ts, line 149 at r2 (raw file):

   * @param predicate A function mapping a stream element to a transformed
   *   element.
   * @param consumePrep A function that tentatively schedules an item or its

instead of consumePrep maybe call this cleanupFn or disposeFn - it took me a little thinking to realize this is simply the callback that gets called at the end to dispose Tensors. Good to keep the same terminology everywhere (I don't see a use case where you'd use consumePrep for non-disposal).
Keeping the "Fn" suffix where you have these functions would be nice, and in general s/retain/keep and s/consume/dispose

src/contrib/data/streams/data_stream.ts, line 154 at r2 (raw file):

   *   function accepts an argument that may modify the list of objects to be
   *   collected, e.g. by reversing the tentative decision made previously.
   * @param retain A function that marks an item to be retained (i.e.,

Also probably call this keepFn for consistency.

src/contrib/data/streams/data_stream.ts, line 170 at r2 (raw file):

   *
   * @param f A function to apply to each stream element.
   * @param consumePrep A function that tentatively schedules an item or its

probably rename this as per above, right now the jsdoc doesn't match the param name

src/contrib/data/streams/data_stream.ts, line 179 at r2 (raw file):

      Promise<void> {
    const consumePrep =
        consume == null ? null : (item: T) => ((output: void) => consume(item));

this looks funny, should this just be
const consumePrep =
      consume == null ? null : (item: T) => () => consume(item);

src/contrib/data/streams/data_stream.ts, line 225 at r2 (raw file):

   * @param count The number of items to skip.  If a negative or undefined value
   *   is given, the entire stream is returned unaltered.
   * @param consume A function that finalizes an item, e.g. by marking it for

probably rename this to disposeFn as well

Comments from Reviewable

Per offline discussion, this solution is much simpler, but trusts that a) the outer scope won't  unexpectedly dispose Tensors, and b) Tensors passed into pipeline stages may be safely disposed, i.e., they have no other usages.

Review status: 0 of 10 files reviewed at latest revision, 14 unresolved discussions, some commit checks failed.

src/contrib/data/batch_dataset_test.ts, line 95 at r1 (raw file):
Previously, nsthorat (Nikhil Thorat) wrote…
I think it's still worth it to try to consolidate. File-level exports will never get exported at the module level unless explicitly told to do so, so I wouldn't worry about extra file-level exports.
Ways to consolidate:

for disposeBatchPrep, it doesn't matter that the function has an argument, you can simply use () => with no args since you don't use the argument (typescript will be happy). Then you can always taken an array of DatasetElements or a single DatasetElement and return that function.
Same for extractTensorsFromElementArray, it can take a single one or an array.

obsoleted by new design.

src/contrib/data/dataset.ts, line 287 at r1 (raw file):
Previously, nsthorat (Nikhil Thorat) wrote…
Awesome. One more small thing, can you s/undefined/null

obsoleted by new design.

src/contrib/data/dataset.ts, line 360 at r1 (raw file):
Previously, davidsoergel (David Soergel) wrote…
See above: similar but not the same, and private.  Happy to move it out if you still prefer it that way.  Another approach would be to graduate DatasetElement and DatasetBatch to full classes, which could then have keep(), dispose(), and disposePrep() methods.

obsoleted by new design.

src/contrib/data/dataset_test.ts, line 19 at r1 (raw file):
Previously, nsthorat (Nikhil Thorat) wrote…
Yeah, we've been slowly migrating to this convention:

Tests import public API as dl through './index' unless they are testing something not module-exported
Internal library code does not import through index (because of cycles), it depends on the files that export our factory functions

SGTM, thanks!

src/contrib/data/dataset_test.ts, line 147 at r2 (raw file):
Previously, nsthorat (Nikhil Thorat) wrote…
why 200? where does the 2 come from?

Added comments to all these tests.

src/contrib/data/dataset_test.ts, line 154 at r2 (raw file):
Previously, nsthorat (Nikhil Thorat) wrote…
Why 170? Can you make it clear here either with a variable or a comment? If I just read this without understanding skip, I would assume this is testing that we have a leak :)
Other places in this file too that you assert a non-zero constant

Done.

src/contrib/data/streams/data_stream.ts, line 150 at r1 (raw file):
Previously, nsthorat (Nikhil Thorat) wrote…
Convention :)

OK :)

src/contrib/data/streams/data_stream.ts, line 149 at r2 (raw file):
Previously, nsthorat (Nikhil Thorat) wrote…
instead of consumePrep maybe call this cleanupFn or disposeFn - it took me a little thinking to realize this is simply the callback that gets called at the end to dispose Tensors. Good to keep the same terminology everywhere (I don't see a use case where you'd use consumePrep for non-disposal).
Keeping the "Fn" suffix where you have these functions would be nice, and in general s/retain/keep and s/consume/dispose

obsoleted by new design.

src/contrib/data/streams/data_stream.ts, line 154 at r2 (raw file):
Previously, nsthorat (Nikhil Thorat) wrote…
Also probably call this keepFn for consistency.

obsoleted by new design.

src/contrib/data/streams/data_stream.ts, line 170 at r2 (raw file):
Previously, nsthorat (Nikhil Thorat) wrote…
probably rename this as per above, right now the jsdoc doesn't match the param name

obsoleted by new design.

src/contrib/data/streams/data_stream.ts, line 179 at r2 (raw file):
Previously, nsthorat (Nikhil Thorat) wrote…
this looks funny, should this just be
const consumePrep =
      consume == null ? null : (item: T) => () => consume(item);

obsoleted by new design.

src/contrib/data/streams/data_stream.ts, line 225 at r2 (raw file):
Previously, nsthorat (Nikhil Thorat) wrote…
probably rename this to disposeFn as well

obsoleted by new design.

Comments from Reviewable

Reviewed 2 of 7 files at r1, 8 of 9 files at r3.
Review status: 10 of 11 files reviewed at latest revision, 25 unresolved discussions, some commit checks failed.

src/engine.ts, line 460 at r3 (raw file):

// This is a copy of extractTensorsFromScopeResult but with no type constraints.
// tslint:disable-next-line:no-any
export function extractTensorsFromAny(result: any): Tensor[] {

Let's consolidate: move extractTensorsFromScopeResult to util.ts and rename to extractTensorsFromContainer and use it in both places. Rename also ScopeResult to TensorContainer, and similarly for the related types.
If you run into type constraints issues, feel free to relax the type, though you may not need to since the current type (ScopeResult) is already general and can describe any primitive/array/JSON object. The only thing that type prohibits is a Promise<any>, rightfully so.

src/tracking.ts, line 103 at r3 (raw file):

  // DO NOT SUBMIT; stopgap.
  // tslint:disable-next-line:no-any
  static dispose(x: any) {

type it as x: TensorContainer (see comment above)

src/tracking.ts, line 105 at r3 (raw file):

  static dispose(x: any) {
    const tensors = extractTensorsFromAny(x);
    for (const t of tensors) {

this is a bit unconventional since tensors is an Array, not an Object, and not supported in IE (pre Edge). Use:
tensors.forEach(t => t.dispose());

for consistency with the rest of the codebase

src/contrib/index.ts, line 20 at r3 (raw file):

// TODO(soergel) carefully consider what actually should be exposed here
export {Dataset} from './data/dataset';
export {datasetFromConcatenated, datasetFromElements} from './data/dataset';

let's not export these yet, since these probably will not be exposed to the user later, and we want to avoid modifying tests later. In your tests, import these 2 methods directly from their file, and import from 'index' only the things that are already exported in index.ts

src/contrib/data/batch_dataset.ts, line 93 at r3 (raw file):

    }
  }
  for (const e of elements) {

if something is an array, use .forEach(e => e.dispose()) for consistency.

src/contrib/data/batch_dataset_test.ts, line 95 at r1 (raw file):
Previously, davidsoergel (David Soergel) wrote…
obsoleted by new design.

Since this is obsolete now that we have dl.dispose(), remove it?

src/contrib/data/batch_dataset_test.ts, line 22 at r3 (raw file):

import {TestDataset} from './dataset_test';

describeWithFlags('Dataset.batch()', CPU_ENVS, () => {

Use ALL_ENVS since we removed the byte packing settings  from ALL_ENVS and things are numerically stable. We will bring the byte-packing setting when we make it numerically stable.

src/contrib/data/batch_dataset_test.ts, line 30 at r3 (raw file):

        .then(batchStream => batchStream.collectRemaining().then(result => {
          expect(result.length).toEqual(13);
          for (const batch of result.slice(0, 12)) {

let's do forEach() for consistency throughout the codebase.

src/contrib/data/batch_dataset_test.ts, line 38 at r3 (raw file):

          return result;
        }))
        .then((result) => {

any reason why we avoid async/await. Is it because jasmine/karma can't catch any error that way?
If that's the case, would this work:
try {
  const a = await someAsyncF();
  ...
} catch (e) {
  done.fail(e);
}

src/contrib/data/dataset.ts, line 54 at r3 (raw file):

   * The dl.tidy() approach cannot be used in a asynchronous context.
   */
  abstract async getStream(): Promise<DataStream<DatasetElement>>;

Not for this PR, but wanted to start the discussion: Can we make getStream sync, or is there a fundamental reason? I want to push for only getNext() be async, and everything else be sync, to the extent possible.

src/contrib/data/dataset_test.ts, line 45 at r3 (raw file):

      'string': `Item ${elementNumber}`
    };
    dl.keep(result['Tensor']);

just double-checking, with the new design, is keep still needed?

src/contrib/data/streams/data_stream.ts, line 356 at r3 (raw file):

// TODO(soergel): consider clean separation of synchronous pumpOne
/*abstract class TransformingQueueStream<I, O> extends QueueStream<O> {

just checking: if commented out code meant be kept here?

Comments from Reviewable

Review status: 10 of 11 files reviewed at latest revision, 25 unresolved discussions, some commit checks failed.

src/tracking.ts, line 103 at r3 (raw file):
Previously, davidsoergel (David Soergel) wrote…
I tried that; the problem is that currently streams can be of any type, e.g. DataStream, and we want to be able to call dispose on any stream element.  I briefly went down the road of DataStream, but stopped because people ought to be able to stream their own custom types.  And, we can't protect the dispose call with "if(x instanceof ScopeResult)" because ScopeResult is just a compile-time type, not a class.  So that's why I went with any here. WDYT?

Oops let me try that again: I meant "e.g. DataStream<T>" and then "down the road of DataStream<T extends ScopeResult>".

Comments from Reviewable

Review status: 6 of 11 files reviewed at latest revision, 25 unresolved discussions, some commit checks failed.

src/engine.ts, line 460 at r3 (raw file):
Previously, davidsoergel (David Soergel) wrote…
I think I need the any version (see below), but how about having extractTensorsFromContainer just call  extractTensorsFromAny (but with added type safety)?  That way we can prefer the Container version in general, and use the any version only when we specifically need it.

(Preparing this part in another PR to be submitted before this one)

src/tracking.ts, line 105 at r3 (raw file):
Previously, dsmilkov (Daniel Smilkov) wrote…
this is a bit unconventional since tensors is an Array, not an Object, and not supported in IE (pre Edge). Use:
tensors.forEach(t => t.dispose());

for consistency with the rest of the codebase

Done.

src/contrib/index.ts, line 20 at r3 (raw file):
Previously, dsmilkov (Daniel Smilkov) wrote…
let's not export these yet, since these probably will not be exposed to the user later, and we want to avoid modifying tests later. In your tests, import these 2 methods directly from their file, and import from 'index' only the things that are already exported in index.ts

I think users are very likely to want these, at least for writing their own tests.  Especially concatenating datasets seems like something the api should clearly provide.

src/contrib/data/batch_dataset.ts, line 93 at r3 (raw file):
Previously, dsmilkov (Daniel Smilkov) wrote…
if something is an array, use .forEach(e => e.dispose()) for consistency.

Done here; I'll keep my eye out for other instances.

src/contrib/data/batch_dataset_test.ts, line 95 at r1 (raw file):
Previously, dsmilkov (Daniel Smilkov) wrote…
Since this is obsolete now that we have dl.dispose(), remove it?

Done

src/contrib/data/batch_dataset_test.ts, line 22 at r3 (raw file):
Previously, dsmilkov (Daniel Smilkov) wrote…
Use ALL_ENVS since we removed the byte packing settings  from ALL_ENVS and things are numerically stable. We will bring the byte-packing setting when we make it numerically stable.

I had changed it from ALL_ENVS to CPU_ENVS at @nsthorat's request, which I think was about saving time when the GPU test is unlikely to reveal any issue anyway (i.e., there's no math in here).  I don't have an opinion either way.  Just tell me what you guys agree ;)

src/contrib/data/batch_dataset_test.ts, line 30 at r3 (raw file):
Previously, dsmilkov (Daniel Smilkov) wrote…
let's do forEach() for consistency throughout the codebase.

+1, thanks, I prefer that too and just forgot

src/contrib/data/dataset.ts, line 54 at r3 (raw file):
Previously, dsmilkov (Daniel Smilkov) wrote…
Not for this PR, but wanted to start the discussion: Can we make getStream sync, or is there a fundamental reason? I want to push for only getNext() be async, and everything else be sync, to the extent possible.

Discussion underway in #798.

src/contrib/data/dataset_test.ts, line 45 at r3 (raw file):
Previously, dsmilkov (Daniel Smilkov) wrote…
just double-checking, with the new design, is keep still needed?

Nope, good catch, thanks!

src/contrib/data/streams/data_stream.ts, line 356 at r3 (raw file):
Previously, dsmilkov (Daniel Smilkov) wrote…
just checking: if commented out code meant be kept here?

Oops no :)

Comments from Reviewable

Review status: 2 of 8 files reviewed at latest revision, 16 unresolved discussions, some commit checks failed.

src/contrib/index.ts, line 20 at r3 (raw file):
Previously, davidsoergel (David Soergel) wrote…
I think users are very likely to want these, at least for writing their own tests.  Especially concatenating datasets seems like something the api should clearly provide.

Can we not deviate from https://www.tensorflow.org/api_docs/python/tf/contrib/data/Dataset#concatenate? I want to make sure there are no redundant methods in the user facing API. LGTM to keep the ball rolling, but let's resolve this comment in private.

src/contrib/data/batch_dataset_test.ts, line 22 at r3 (raw file):
Previously, davidsoergel (David Soergel) wrote…
I had changed it from ALL_ENVS to CPU_ENVS at @nsthorat's request, which I think was about saving time when the GPU test is unlikely to reveal any issue anyway (i.e., there's no math in here).  I don't have an opinion either way.  Just tell me what you guys agree ;)

Got it. CPU is fine then, This assumes our GPU works, and that's a fair assumption. Core can worry about tht.

Comments from Reviewable

Review status: 1 of 8 files reviewed at latest revision, 16 unresolved discussions, some commit checks pending.

src/contrib/index.ts, line 20 at r3 (raw file):
Previously, dsmilkov (Daniel Smilkov) wrote…
Can we not deviate from https://www.tensorflow.org/api_docs/python/tf/contrib/data/Dataset#concatenate? I want to make sure there are no redundant methods in the user facing API. LGTM to keep the ball rolling, but let's resolve this comment in private.

Done

Comments from Reviewable


drawing target

Hi there,
We are interested in supporting Godot, just lacking manpower right now. I see that you are using HWND for the drawing target. You may find https://github.com/videolan/libvlcsharp/blob/master/samples/LibVLCSharp.CustomRendering.Direct3D11/Program.cs interesting (the Unity integration uses these libvlc APIs, from C++ though).
Feel free to come say hi on our discord. Cheers!

Neat! C++ Might be a better integration for Godot anyway, as it would be able to be incorporated with a standard Godot build, as opposed to needing to be the mono version. I'll for sure drop by the discord. Thanks!

👍


Can't log in

Http status -1 error when trying to log in with admin or demo user.
RHEL 6.8
NPM version 2.15.11
Node v4.7.0

angular.js:14195 TypeError: Cannot read property 'user' of undefined
    at Object.authorize (NodesService.js:27)
    at stateChangeStart (app.js:179)
    at Scope.$broadcast (angular.js:18316)
    at transitionTo (angular-ui-router.js:3140)
    at Object.$delegate.transitionTo (app.js:41)
    at Array.<anonymous> (angular-ui-router.js:2318)
    at Object.invoke (angular.js:4767)
    at handleIfMatch (angular-ui-router.js:1822)
    at angular-ui-router.js:1877
    at check (angular-ui-router.js:1992)
(anonymous) @ angular.js:14195
http://<IP_ADDRESS>:1338/__getcookie Failed to load resource: net::ERR_CONNECTION_REFUSED
angular.js:14195 TypeError: Cannot read property 'user' of undefined
    at Object.authorize (NodesService.js:27)
    at stateChangeStart (app.js:179)
    at Scope.$broadcast (angular.js:18316)
    at transitionTo (angular-ui-router.js:3140)
    at Object.$delegate.transitionTo (app.js:41)
    at Array.<anonymous> (angular-ui-router.js:2318)
    at Object.invoke (angular.js:4767)
    at handleIfMatch (angular-ui-router.js:1822)
    at angular-ui-router.js:1877
    at check (angular-ui-router.js:1992)
(anonymous) @ angular.js:14195
angular.js:14195 TypeError: Cannot read property 'user' of undefined
    at Object.authorize (NodesService.js:27)
    at stateChangeStart (app.js:179)
    at Scope.$broadcast (angular.js:18316)
    at transitionTo (angular-ui-router.js:3140)
    at Object.$delegate.transitionTo (app.js:41)
    at Array.<anonymous> (angular-ui-router.js:2318)
    at Object.invoke (angular.js:4767)
    at handleIfMatch (angular-ui-router.js:1822)
    at angular-ui-router.js:1877
    at check (angular-ui-router.js:1992)
(anonymous) @ angular.js:14195

Accidentally used "Close and comment".  Issue is still open.

Hum, I just installed Konga following all the steps to put the app into production and, when attempting to login, I got the same messages:

at vendors.min.js:5:

TypeError: Cannot read property 'user' of undefined
    at Object.authorize (frontend.min.js:1)
    at frontend.min.js:3
    at h.$broadcast (vendors.min.js:6)
    at C.transitionTo (vendors.min.js:10)
    at Object.e.transitionTo (frontend.min.js:3)
    at Array.<anonymous> (vendors.min.js:10)
    at Object.a [as invoke] (vendors.min.js:4)
    at a (vendors.min.js:10)
    at vendors.min.js:10
    at e (vendors.min.js:10)

at vendors.min.js:19:

GET http://localhost:1338/__getcookie net::ERR_CONNECTION_REFUSED

at vendors.min.js:5:

OPTIONS http://localhost:1338/login net::ERR_CONNECTION_REFUSED

NOTE: I'm running Konga in production mode and trying to accessing it via a FQDN, through my browser, Google Chrome Version 55.0.2883.95 (64-bit) on Mac OSX El Capitan.

As far as i can see, i think your issues are network related.
ERR_CONNECTION_REFUSED
It seems that the frontend can't access localhost:1338
Maybe try to change 'localhost' with your computer's IP address?
Does this issue occurs on development mode as well?

I had same problem. However when I changed below setting, I could access it!
at ./konga/frontend/config/config.json :line2
before :   "backendUrl": "http://localhost:1338",
after    :   "backendUrl": "http://<my server IP address>:1338",
thx @pantsel

It works after changing the setting.
Thank you guys! 😄
I guess we can close this issue?

Closing this issue as it seems to be have been resolved.


Implement/adapt E2E test case covering Podman installation via Podman Desktop on Mac

We need to implement or adapt e2e test case to accommodate podman installation via podman desktop on Mac environment.

This is already covered by podman-desktop-e2e at: https://github.com/containers/podman-desktop-e2e/blob/87278c3f9ae38ed3810f8b2bcb16c02197c70dde/test/extended/podman-desktop/extension/podman/podman-extension_darwin.go#L35
We just need to provision the mac machine and set the envs on the podman-desktop-e2e container accordingly

This works out of a box actually thanks to @adrianriobo support in goax.


Array removeAll() is conflicting with Swift implementation

When I call removeAll() of an array I get "Ambiguous use of 'removeAll' because it has implementation on the Swift.Array and EZSwiftExtensions.Array.
Example code is attached to the issue.
RemoveAllTest.zip

So this happens because removeAll in the EZSwift land takes variadic parameters. This causes conflict in the special case in which the num args = 0.
I added the condition that at least one arg needs to be passed to removeAll and test cases for the same. https://github.com/goktugyil/EZSwiftExtensions/pull/436

@lfarah @goktugyil Can you guys look at this, this is a bug so its high priority.

Merged the change. Resolving now.

A quick fix I did was do:
array = []


Run builds in parallel on GHA

Description
Previous implementation of makefile for cli prevents multiple jobs.
Now we should get significant speedup by passing -j option.

Current dependencies on/for this PR:

master

PR #4018

PR #4019   👈

This comment was auto-generated by Graphite.

OSCI images built in 12 minutes 😱 https://prow.ci.openshift.org/view/gs/origin-ci-test/pr-logs/pull/stackrox_stackrox/4019/pull-ci-stackrox-stackrox-master-images/1599804090114641920

/retest


IValueConverter cannot convert to value indirectly inherited from DependencyObject

Describe the bug

When converting a databound value to an instance indirectly inheriting from DependencyObject the following data binding error occurs:
Error    1    WinUI3ConverterRepro.MainWindowItem    Level    WinUI3ConverterRepro.MainWindowItem.Configuration    Level1Configuration    Converter failed to convert value of type 'WinUI3ConverterRepro.Level2Configuration' to type 'Level1Configuration'
Thus we are not able to bind to the converter result.
Steps to reproduce the bug

Start the following repro app. Binding errors will be displayed in the output window.
WinUI3ConverterRepro_2020-11-23.zip
The issue is caused by using DependencyObject as base class:
// Issue is caused by using DependencyObject as base class
// TODO: Remove base class to get expected behavior.
public class Level1Configuration : DependencyObject
{
  public string Label { get; set; }
}

public class Level2Configuration : Level1Configuration
{
  public string IconName { get; set; }
}

public class LevelToConfigurationConverter : IValueConverter
{
  public object Convert(object value, Type targetType, object parameter, string language)
  {
    int level = (int)value;
    switch (level)
    {
      case 1:
        return new Level1Configuration { Label = "Created by Converter: One" };
      case 2:
        return new Level2Configuration { Label = "Created by Converter: Two", IconName = "Two" };
      default:
        throw new NotSupportedException();
     }
  }

  public object ConvertBack(object value, Type targetType, object parameter, string language)
  {
    throw new NotSupportedException();
  }
}

Expected behavior

We are able to bind to the converter result in XAML.
Screenshots

Version Info

NuGet package version:
[Microsoft.WinUI 3.0.0-preview3.201113.0]

Windows app type:

UWP
Win32

Yes
Yes

Windows 10 version
Saw the problem?

Insider Build (xxxxx)

November 2020 Update (19042)
Yes

May 2020 Update (19041)

November 2019 Update (18363)

May 2019 Update (18362)

October 2018 Update (17763)

April 2018 Update (17134)

Fall Creators Update (16299)

Creators Update (15063)

Device form factor
Saw the problem?

Desktop
Yes

Xbox

Surface Hub

IoT

Additional context

Issue is not stale. I just updated to the latest stable and also preview (1.4.230628000-preview1) versions.
Please see updated and simplified repro:
WinUI3ConverterRepro.zip
Changing base class from DependencyObject to INotifyPropertyChanged in file LevelConfigurationBase fixes the problem.


[Feature Request] Custom mapping of battery percentage in PSM devices

It would be nice to have the capability to map 0% and 100% of battery percentage to custom voltages so that different types of batteries can be properly monitored in PSM firmware.
I believe this should be easy to implement in the GUI.

The firmware can report battery voltage. You can handle it in a higher level system and create the necessary mapping.


JAXB xjc CClassInfo constructor with fullname constructs wrong package

CClassInfo has the below constructor which takes a JCodeModel and a fullName. The fullName parameter should be the FQCN of the class. However, if the fullName starts with a package having more than one name segment (like "com.example" which has two segments), the following code fails, because it puts the resulting CClassInfo into the package that is the first segment.
Assuming a fullName "com.example.MyClass", the code below will find that the indexOf '.' is 3, and from there everything goes wrong.
The constructor sets this.parent to the package "com" and it will use fullName.substring(idx+1) to determine the shortName, i.e. he shortName ends up being "example.MyClass".
**com.sun.tools.xjc.model.CClassInfo**
public CClassInfo(Model model,JCodeModel cm, String fullName, Locator location, QName typeName, QName elementName, XSComponent source, CCustomizations customizations) {
        super(model,source,location,customizations);
        this.model = model;
        int idx = fullName.indexOf('.');
        if(idx<0) {
            this.parent = model.getPackage(cm.rootPackage());
            this.shortName = model.allocator.assignClassName(parent,fullName);
        } else {
            this.parent = model.getPackage(cm._package(fullName.substring(0,idx)));
            this.shortName = model.allocator.assignClassName(parent,fullName.substring(idx+1));
        }
        this.typeName = typeName;
        this.elementName = elementName;

        model.add(this);
    }

The line
int idx = fullName.indexOf('.');
should be
int idx = fullName.lastIndexOf('.');
Source: https://github.com/javaee/metro-jax-ws/issues/1187
Author: glassfishrobot

Fixed in  748571a. Maybe could you close it, @lukasj ?
Regards,
Antonio.


OpenStreetMap Indonesia - Telegram Group

Community Resource Name
OpenStreetMap Indonesia - Telegram

What is it?
OpenStreetMap Indonesia Telegram group

URL link to the resource
https://t.me/osmindonesia

Location
Indonesia

Description (optional)

Languages spoken (optional)
Indonesia

Points of contact (optional)

Resolved with d2da398


Reduce binary size by leaving out artifacts

Takes a good chunk out of #319.
Fixes #1153.
Fixes https://github.com/Deltares/Ribasim/pull/687#issuecomment-1772635668
Our ribasim_cli_windows.zip is 535 MB on main, this reduces it to 227 MB.
Unpacked, it goes from 1.9 GB to 1.0 GB.
Libribasim shrinks by the same amount.
This is because we no longer include two 400 MB artifacts, MinGW and MKL. Three PackageCompiler keyword arguments come into play here:

set to true: include_preferences::Bool: If true, store all preferences visible by the project in package_dir in the app bundle. Defaults to true.
set to false: include_lazy_artifacts::Bool: if lazy artifacts should be included in the bundled artifacts, defaults to false.
set to false: include_transitive_dependencies::Bool: If true, explicitly put all transitive dependencies into the sysimage. This only makes a difference if some packages do not load all their dependencies when themselves are loaded. Defaults to true.

I added an explicit include_preferences=true. This is mostly to highlight that we are making use of Preferences to tell LinearSolve that we don't want to load MKL_jll. This would be enough outside of PackageCompiler, however more was needed. The include_lazy_artifacts=false is restoring the default behavior. We had set it to true to stop it from downloading MKL_jll the first time the CLI was used. When I added this option, it would still do that, so more was needed. The include_transitive_dependencies=false is deviating from the default, but this causes several issues. I'm not sure, but for MKL_jll perhaps the problem was that the code was already fixed to the default load MKL_jll behavior in the sysimage.
One more note about choosing to opt out of using MKL_jll, by default we use KLUFactorization, since in [solver] we set sparse = true by default. It could be that sparse = false will be a bit slower. I haven't taken the time to measure, but I doubt it is worth the binary size, since sparse = false only makes sense for small models.
A nice side effect is that this combination of keys also took MinGW out of the sysimage. The fact that we got it might be a PackageCompiler bug, though I could not repoduce this in small test executables.
Some references for those interested:
https://julialang.github.io/PackageCompiler.jl/stable/refs.html#PackageCompiler.create_app
https://juliapackaging.github.io/Preferences.jl/dev/
https://github.com/SciML/LinearSolve.jl/pull/425/files
I snuck in some extra preferences. When precompiling, OrdinaryDiffEq always takes very long since it precompiles a lot of combinations of solvers and specializations. I had a look at the docs and precompilation code linked below, and concluded that we probably don't gain too much from these precompile workloads. Our default solver, QNDF, is not precompiled, and for snappy CLI startup we have our own precompilation script that runs Ribasim, which is probably more effective anyway. That means developers using Julia have to spend less time waiting for OrdinaryDiffEq to precompile, possibly at the cost of waiting slightly longer to compile a first Ribasim run from Julia. I haven't measured. Another benefit is that we will create less code; the lib/julia/sys.dll shrinks by 50 MB to 672 MB due to less unused OrdinaryDiffEq code.
https://docs.sciml.ai/DiffEqDocs/stable/features/low_dep/
https://github.com/SciML/OrdinaryDiffEq.jl/blob/v6.71.0/src/OrdinaryDiffEq.jl#L236-L334
https://github.com/trixi-framework/libtrixi/blob/v0.1.4/LibTrixi.jl/Project.toml
The builds from this branch do run the test models just fine, don't download anything on start, and start more quickly.

nice work !


Force unix line endings from windows devs

I'm on windows and I don't want to push files that use windows line
endings so I'm adding this setting. I have other repositories that
demand windows line endings so it's best to specify in the repo and not
rely on user settings.

if you want to test and make sure you'll still get unix on your side after i edit a file, check out these two files where I made a change to a unix file and committed it and it stayed unix line endings:
https://github.com/robertpateii/habitica/blob/test/README.md
https://github.com/robertpateii/habitica/blob/test/Makefile


Makefile/Application: correct libgcc PATH

Summary
Makefile/Application: correct libgcc PATH
if "--print-libgcc-file-name" unable to find the correct libgcc PATH
then go ahead and try "--print-file-name"
Impact
N/A
Testing
ToolChain:
https://github.com/ARM-software/LLVM-embedded-toolchain-for-Arm
./tools/configure.sh lm3s6965-ek:qemu-flat
CONFIG_ARMV7M_TOOLCHAIN_CLANGL=y

Depends on : https://github.com/apache/incubator-nuttx/pull/6076

Testing
ToolChain: https://github.com/ARM-software/LLVM-embedded-toolchain-for-Arm

what's the output of --print-libgcc-file-name and --print-file-name for the target?


[Snyk] Upgrade @spectrum-web-components/theme from 0.44.0 to 0.45.0

Snyk has created this PR to upgrade @spectrum-web-components/theme from 0.44.0 to 0.45.0.
:information_source: Keep your dependencies up-to-date. This makes it easier to fix existing vulnerabilities and to more quickly identify and fix newly disclosed vulnerabilities when they affect your project.

The recommended version is 1 version ahead of your current version.

The recommended version was released on 22 days ago.

Release notes

Package name: @spectrum-web-components/theme

0.45.0 - 2024-07-30v0.45.0

0.44.0 - 2024-07-15v0.44.0

    from @spectrum-web-components/theme GitHub release notes

[!IMPORTANT]

Check the changes in this PR to ensure they won't cause issues with your project.
This PR was automatically created by Snyk using the credentials of a real user.

Note: You are seeing this because you or someone else with access to this repository has authorized Snyk to open upgrade PRs.
For more information:

🧐 View latest project report
📜 Customise PR templates
🛠 Adjust upgrade PR settings
🔕 Ignore this dependency or unsubscribe from future upgrade PRs

Run report for 49cc85fd
Total time: 2.9s | Comparison time: 2.9s | Estimated loss: 4.9ms (0.2% slower)

Action
Time
Status
Info

🟩
SyncWorkspace
4.7ms
Passed

⬛
SetupToolchain(node:~20.12)
1.7s
Skipped

🟩
SyncProject(node, visualizer)
0.2ms
Passed

🟩
SyncProject(node, root)
0.2ms
Passed

🟥
InstallWorkspaceDeps(node:~20.12)
1.2s
Aborted

Touched files
docs/visualizer/package.json


USDC [BNB -> ETH] stuck for > 3hrs

Problem
https://connextscan.io/tx/0xca64331fc99ebeb3cace1a33b76fbe23718c463a277dc28f53e6d156c2b7fa2f
Raised by socket team

It should be ideally completed by multipath, p2p has 11K and 0xc4ae07f276768a3b74ae8c47bc108a2af0e40eba has around 5k and few other routers has more several thousands.

In worst case, for mainnet-destination transfers slowpath takes up to 12hrs.

completed


CheckingResponse proto does not match API response

This is what the checkOrder endpoint returns for me. If I try to create an order I get the error that transactionAutoFxSurcharges is not part of the CheckingResponse proto. I fixed this for myself by matching the proto to the API response.
I have a custody account.

Hi,
thanks for reporting that bug.
I was able to reproduce this issue.
The format of the CheckingResponse returned by Degiro's API seems to depend on the product.
I have updated the library in order to :

make the CheckingResponse  less restrictive
parse both formats properly

By the way there is a raw parameter in most of the methods in order to get the raw API response:
checking_response = trading_api.check_order(order=order, raw=True)

Awesome, thanks.


Class Summary, Free Response Tab Breadcrumbs scroll up with page content

The breadcrumbs menu on the right (that say "Stage 4" "Stage 5" etc should stay fixed on the screen and remain in view when the user scrolls through the free responses. Instead, they are fixed to the top of the Stage 4 content, so when the user scrolls up, the menu disappears.

This was implemented in #36 in 11f3f677a2c956e00a45b6ed259e18d5c1a2fef8


Token validation

Dear angular-oauth2-oidc team,
Is the retrieved token automatically validated when retrieved? I mean validity and signature?
Or is it something we have to do on our own?
I'm using implicit flow.
Thanks a lot
Regards
Maxime

For v9 check this guidance on token validation with the implicit flow.
For versions below that, provide the JwksValidationHandler to enable validation if your app requires it.
Of course, servers (APIs) should do their own validation of the tokens.


Abandoned project?

@vinlyx I believe that you may have abandoned this project. We haven't heard from you in well over a year and have multiple changes we would like to get merged and pushed into pypi.
I have a few people including myself that are willing to take over as maintainers. I have created a organisation at https://github.com/mgzip with the intention of making this a community maintained project.
If you transfer the project to my personal account @timhughes I can then do the shuffle to replace the version of mgzip that i have forked in https://github.com/mgzip/mgzip
We would also like to take over the maintenance of the https://pypi.org/project/mgzip/
To do this we need you to log in, go to the “Collaborators” section in the settings (https://pypi.org/manage/project//collaboration/), add timhughes as Owner. I can then can then remove you from the project if you wish.

Forked as https://pypi.org/project/pgzip/ https://github.com/pgzip/pgzip