text:
[BUG]  Performance Issues reading LocalDate,LocalDateTime, and LocalTime

Driver version
7.2.1.jre8
SQL Server version
Microsoft SQL Server 2014 (SP3) (KB4022619) - 12.0.6024.0 (X64)
Sep  7 2018 01:37:51
Copyright (c) Microsoft Corporation
Developer Edition (64-bit) on Windows NT 6.3  (Build 9600: )
Client Operating System
Ubuntu 16.04
JAVA/JVM version
openjdk version "12" 2019-03-19
OpenJDK Runtime Environment (build 12+33)
OpenJDK 64-Bit Server VM (build 12+33, mixed mode, sharing)
Table schema
CREATE TABLE test.DatePerformance (
	someDate DATE
)

-- Populate Table with Random Dates
INSERT INTO test.DatePerformance
SELECT DATEADD(DAY, -RAND(12313456) * 1000, GETDATE())

WHILE((SELECT COUNT(*) FROM test.DatePerformance) < 2500)
INSERT INTO test.DatePerformance
SELECT DATEADD(DAY, -RAND() * 1000, GETDATE())

Problem description

Expected behaviour:

Reading Date objects from the database should have low overhead.

Actual behaviour:

There is substantial overhead creating GregorianCalendars and converting them to the LocalDate, LocalDateTime, LocalTime classes
Calling Calendar.getInstance(TimeZone.getTimeZone("UTC")) is 30% of the getObject call for LocalDate, LocalDateTime, LocalTime classes

Error message/stack trace:

Any other details that can be helpful:

Result Set - Calendar.getInstance https://github.com/microsoft/mssql-jdbc/blob/v7.2.1/src/main/java/com/microsoft/sqlserver/jdbc/SQLServerResultSet.java#L2384-L2398
DDC.convertTemporalToObject - https://github.com/microsoft/mssql-jdbc/blob/v7.2.1/src/main/java/com/microsoft/sqlserver/jdbc/DDC.java#L755-L1061

Sample Profiling Snapshots

	com.microsoft.sqlserver.jdbc.SQLServerResultSet.getObject () 15.094s
>	com.microsoft.sqlserver.jdbc.DDC.convertTemporalToObject () 9.206s
>>	java.util.GregorianCalendar.<init> () 6.01s
>>	java.util.Calendar.getTimeInMillis () 3.005s
>	java.util.Calendar.getInstance () 5.088s

	com.microsoft.sqlserver.jdbc.SQLServerResultSet.getObject () 7.601s
>	com.microsoft.sqlserver.jdbc.DDC.convertTemporalToObject () 3.906s
>>	java.util.GregorianCalendar.<init> () 2.498s
>>	java.util.Calendar.getTimeInMillis () 1.407s
>	java.util.Calendar.getInstance () 3.302s

Reproduction code
    /**
    Using JMH (LocalDate)

    Result: 338.352 ±(99.9%) 27.177 ops/s [Average]
    Statistics: (min, avg, max) = (5.716, 338.352, 435.926), stdev = 115.069
    Confidence interval (99.9%): [311.175, 365.529]

    # Run complete. Total time: 00:08:10

    Benchmark                        Mode  Samples    Score  Score error  Units
    c.c.m.MyBenchmark.testMethod    thrpt      200  338.352       27.177  ops/s
    */

    @Benchmark
    public void testMethod()
    {
        List<LocalDate> dates = new ArrayList<>();

        try (
                Connection conn = dataSource.getConnection();
                PreparedStatement ps = conn.prepareStatement("SELECT someDate from test.DatePerformance");
                ResultSet rs = ps.executeQuery()
        )
        {
            while(rs.next())
                dates.add(rs.getObject("someDate", LocalDate.class));

        } catch (SQLException e)
        {
            e.printStackTrace();
        }
    }

Hi @lavabear, the team will look into this.

@ulvii (et al.) - While investigating this you may also want to consider #1088

If creating Calendar objects is particularly expensive then perhaps a SQLServerResultSet object should have a single instance of a proleptic GregorianCalendar for UTC that can be used for conversions.
(BTW, this is not a bug. It is an enhancement request.)

@gordthompson Care should be taken in the fact that the Calendar object is not thread safe.
Ideally, the driver would switch away from using the calendar object and instead look at using ZonedDateTime or straight local dates.  If my reading of the TDS standard is correct, than a SQL "Date" ends up on the wire as the integer "number of days since 1-1-1"  That would be a trivial conversion to localDates because it would simply be "LocalDate.of(1,1,1).addDays(tdsIntValue);"  (or, whatever that API is.
The new system is much faster and thread safe to boot.  Converting to java.sql.Date is also pretty simple because it is just a "Date.from(ZonedDateTime.toInstant())" away.

There is a handy method microsoft.sql.DateTimeOffset#getOffsetDateTime() added via #830 .
https://github.com/microsoft/mssql-jdbc/blob/v7.3.1/src/main/java/microsoft/sql/DateTimeOffset.java#L222-L227
It would be better to use it for LocalDate/Time. Something like...
} else if (type == java.time.LocalDateTime.class) {
  microsoft.sql.DateTimeOffset dateTimeOffset = getDateTimeOffset(columnIndex);
  if (dateTimeOffset == null) {
    returnValue = null;
  } else {
    returnValue = dateTimeOffset.atZoneSameInstant(ZoneOffset.UTC).toLocalDateTime();
  }

@cogman - Good points. If the driver now requires Java_8+ then datetime2 values should really be unpacked from the TDS payload as LocalDateTime objects (because that's what they really are) and then converted to java.sql.Timestamp values if needed.

Hi @lavabear, @gordthompson , @cogman , @harawata ,
Thank you for the suggestions and sorry for the delay, the team is currently busy with the release preparations.
I went through the discussion briefly and wanted to clarify a few things.

Please note that most of the conversions between SQL Server temporal types and JAVA object types are currently handled using GregorianCalendar, both when reading and writing. Are you proposing to completely migrate away from GregorianCalendar to ZonedDateTime/ LocalDateTime or only for LocalDate, LocalDateTime, and LocalTime classes when calling getObject()?
Do the new ZonedDateTime/LocalDateTime APIs provide all the functionality that GregorianCalendar has? Are you aware of any limitations?
Are there benchmarks? Do the new APIs always perform better or there are scenarios where we might have to introduce performance regressions?

To speed up the process, please feel free to create an initial PR or post any other recommendations that we should keep in mind when investigating the proposal.

re: performance
Here's a simple benchmark to compare the results of unpacking a datetime2 value.
// Test Data - TDS protocol delivers datetime2 values as two integers:
//   1) the number of days since 0001-01-01, and
//   2) the number of 100-nanosecond "ticks" since the beginning of the current day
Long dateValue = 737240L;        // 2019-07-01
Long timeValue = 432000000000L;  // 12:00:00
//
int loopLimit = 1000;
Long t0 = null;  // loop timer start time

// the old way
t0 = System.nanoTime();
GregorianCalendar cal = new GregorianCalendar(new SimpleTimeZone(0, "UTC"), Locale.US);
java.util.Date foreverAgo = new java.util.Date(Long.MIN_VALUE);
ZoneId utcZone = ZoneId.of("UTC");
for (int i = 0; i < loopLimit; i++) {
    cal.clear();
    cal.setGregorianChange(foreverAgo);  // proleptic
    cal.set(1, 0, 1, 0, 0, 0);  // 0001-01-01
    cal.add(GregorianCalendar.DAY_OF_YEAR, dateValue.intValue());
    cal.add(GregorianCalendar.MILLISECOND, (int) (timeValue / 10000L));
    LocalDateTime theOldWay = LocalDateTime.ofInstant(cal.toInstant(), utcZone);
}
System.out.printf("(old) java.util: %d ms for %d iterations%n",
        (System.nanoTime() - t0) / 1000000,
        loopLimit);
// (old) java.util: 279 ms for 1000 iterations

// the new way
t0 = System.nanoTime();
for (int i = 0; i < loopLimit; i++) {
    LocalDateTime theNewWay = java.time.temporal.ChronoUnit.DAYS.addTo(
            java.time.temporal.ChronoUnit.NANOS.addTo(
                    dt2Base,
                    timeValue * 100L),
            dateValue);
}
System.out.printf("(new) java.time: %d ms for %d iterations%n",
        (System.nanoTime() - t0) / 1000000,
        loopLimit);
// (new) java.time: 23 ms for 1000 iterations

I ran the test several times and the comments showing the console output are representative values from my (very old) test machine.

@ulvii
Here are some answers to your question

Yes.  The Gregorian calendar has a lot of design flaws.  One of the largest being that it is not thread safe (completely mutable) and is fairly slow because it does a ton of excess work on initialization.  That could be a large change, so I'm ok with it waiting.

Yes they provide all the same functionality while also being much closer aligned with how you want to think about dates.  The old API tried to hide away when it was doing things like time zone conversions for you.  The new API is much better at keeping that in plain site.  Further, the old API was highly mutable in often surprising ways.  The new API is completely immutable.  Finally, converting between old and new is relatively simple.

Here are some JMH results based on @gordthompson 's benchmark using Java 11.

MyBenchmark.test_read_local_date_new                                  thrpt   25  19109523.054 ± 136645.875  ops/s MyBenchmark.test_read_local_date_new_calendar_current_implementation  thrpt   25   3235636.892 ±  30089.733  ops/s MyBenchmark.test_read_local_date_reuse_calendar                       thrpt   25   3254206.917 ±  51282.245  ops/s
Validates his claim of being around 5->6x faster.
`package org.sample;
import org.openjdk.jmh.annotations.Benchmark;
import org.openjdk.jmh.annotations.Scope;
import org.openjdk.jmh.annotations.State;
import org.openjdk.jmh.infra.Blackhole;
import java.time.LocalDateTime;
import java.time.ZoneId;
import java.util.*;
@State(Scope.Thread)
public class MyBenchmark {
private static long dateValue = 737240L;
private static long timeValue = 432000000000L;
private static LocalDateTime EPOCH = LocalDateTime.of(1, 1, 1, 12, 0);
private GregorianCalendar cal = new GregorianCalendar(new SimpleTimeZone(0, "UTC"), Locale.US);
private java.util.Date foreverAgo = new java.util.Date(Long.MIN_VALUE);
private ZoneId utcZone = ZoneId.of("UTC");

@Benchmark
public void test_read_local_date_new_calendar_current_implementation(Blackhole bh) {
	bh.consume(makeReusingCalendar());
}

private LocalDateTime makeNewCalendar() {
	GregorianCalendar cal = new GregorianCalendar(new SimpleTimeZone(0, "UTC"), Locale.US);
	java.util.Date foreverAgo = new java.util.Date(Long.MIN_VALUE);
	ZoneId utcZone = ZoneId.of("UTC");

	cal.clear();
	cal.setGregorianChange(foreverAgo);  // proleptic
	cal.set(1, 0, 1, 0, 0, 0);  // 0001-01-01
	cal.add(GregorianCalendar.DAY_OF_YEAR, (int)dateValue);
	cal.add(GregorianCalendar.MILLISECOND, (int) (timeValue / 10000L));
	return LocalDateTime.ofInstant(cal.toInstant(), utcZone);
}

@Benchmark
public void test_read_local_date_reuse_calendar(Blackhole bh) {
	bh.consume(makeReusingCalendar());
}

private LocalDateTime makeReusingCalendar() {

	cal.clear();
	cal.setGregorianChange(foreverAgo);  // proleptic
	cal.set(1, 0, 1, 0, 0, 0);  // 0001-01-01
	cal.add(GregorianCalendar.DAY_OF_YEAR, (int)dateValue);
	cal.add(GregorianCalendar.MILLISECOND, (int) (timeValue / 10000L));
	return LocalDateTime.ofInstant(cal.toInstant(), utcZone);
}

@Benchmark
public void test_read_local_date_new(Blackhole bh) {
	bh.consume(EPOCH.plusDays(dateValue).plusNanos(timeValue * 100));
}

}`

Thanks to @cogman for confirming that unpacking the datetime2 TDS payload directly to LocalDateTime is a pretty big win ...
... if we ultimately want a LocalDateTime value.
But if we ultimately want a java.sql.Timestamp value then unpacking to LocalDateTime and converting to Timestamp appears to take about 40% longer than unpacking to a GregorianCalendar and converting to Timestamp, even if we use the shortcut of arbitrarily using UTC for the Timestamp time zone:
// Test Data - TDS protocol delivers datetime2 values as two integers:
//   1) the number of days since 0001-01-01, and
//   2) the number of 100-nanosecond "ticks" since the beginning of the current day
Long dateValue = 737240L;        // 2019-07-01
Long timeValue = 432000000000L;  // 12:00:00
//
int loopLimit = 1000;
Long t0 = null;  // loop timer start time

// the old way
t0 = System.nanoTime();
GregorianCalendar cal = new GregorianCalendar(new SimpleTimeZone(0, "UTC"), Locale.US);
java.util.Date foreverAgo = new java.util.Date(Long.MIN_VALUE);
ZoneId utcZone = ZoneId.of("UTC");
for (int i = 0; i < loopLimit; i++) {
    cal.clear();
    cal.setGregorianChange(foreverAgo);  // proleptic
    cal.set(1, 0, 1, 0, 0, 0);  // 0001-01-01
    cal.add(GregorianCalendar.DAY_OF_YEAR, dateValue.intValue());
    cal.add(GregorianCalendar.MILLISECOND, (int) (timeValue / 10000L));
    java.sql.Timestamp tsOld = new java.sql.Timestamp(cal.getTimeInMillis());
}
System.out.printf("(old) java.util: %d ms for %d iterations%n",
        (System.nanoTime() - t0) / 1000000,
        loopLimit);
// (old) java.util: 251 ms for 1000 iterations

// the new way
t0 = System.nanoTime();
LocalDateTime dt2Base = LocalDateTime.of(1, 1, 1, 0, 0);
for (int i = 0; i < loopLimit; i++) {
    LocalDateTime theNewWay = java.time.temporal.ChronoUnit.DAYS.addTo(
            java.time.temporal.ChronoUnit.NANOS.addTo(
                    dt2Base,
                    timeValue * 100L),
            dateValue);
    java.sql.Timestamp tsNew = java.sql.Timestamp.from(theNewWay.toInstant(ZoneOffset.UTC));
}
System.out.printf("(new) java.time: %d ms for %d iterations%n",
        (System.nanoTime() - t0) / 1000000,
        loopLimit);
// (new) java.time: 360 ms for 1000 iterations

If we wanted our Timestamp to be in the default time zone for the JVM then presumably we'd have to go from LocalDateTime through java.time.ZonedDateTime (to account for the changing offsets in Standard vs. Daylight/Summer time), and that might be even slower.
However, continuing to get a LocalDateTime via GregorianCalendar might not be advisable anyway because GregorianCalendar is limited to millisecond resolution, thereby making it impossible(?) to retrieve a LocalDateTime object with the full precision of a datetime2(x) value where x>3.

@gordthompson I believe your benchmark might be falling prey to some common java microbenchmarking problems. (No warmup, not consuming the result, etc).  If I were to guess, your method isn't getting fully optimized since it isn't "hot" enough.
I've ran your scenario through JMH tests and I'm getting different results (but not really surprising).

MyBenchmark.test_read_local_date_new             thrpt   25  14836448.410 ± 181472.322  ops/s
MyBenchmark.test_read_local_date_reuse_calendar  thrpt   25   3322991.970 ±  97922.281  ops/s

It isn't surprising because the 2 extra steps to create a Timestamp boil down to a couple of additions and multiplications.  The calendar code has a ton of extra checks.  It is pretty intense when you start reading everything that goes into "getTimeInMillis" on the cal object.
Here is the code I benchmarked with
package org.sample;

import org.openjdk.jmh.annotations.Benchmark;
import org.openjdk.jmh.annotations.Scope;
import org.openjdk.jmh.annotations.State;
import org.openjdk.jmh.infra.Blackhole;

import java.sql.Timestamp;
import java.time.LocalDateTime;
import java.time.ZoneId;
import java.time.ZoneOffset;
import java.util.*;

@State(Scope.Thread)
public class MyBenchmark {
	private static long dateValue = 737240L;
	private static long timeValue = 432000000000L;
	private static LocalDateTime EPOCH = LocalDateTime.of(1, 1, 1, 12, 0);

	private GregorianCalendar cal = new GregorianCalendar(new SimpleTimeZone(0, "UTC"), Locale.US);
	private java.util.Date foreverAgo = new java.util.Date(Long.MIN_VALUE);
	private ZoneId utcZone = ZoneId.of("UTC");

	@Benchmark
	public void test_read_local_date_reuse_calendar(Blackhole bh) {
		bh.consume(makeReusingCalendar());
	}

	private Timestamp makeReusingCalendar() {

		cal.clear();
		cal.setGregorianChange(foreverAgo);  // proleptic
		cal.set(1, 0, 1, 0, 0, 0);  // 0001-01-01
		cal.add(GregorianCalendar.DAY_OF_YEAR, (int)dateValue);
		cal.add(GregorianCalendar.MILLISECOND, (int) (timeValue / 10000L));
		return new Timestamp(cal.getTimeInMillis());
	}

	@Benchmark
	public void test_read_local_date_new(Blackhole bh) {
		bh.consume(Timestamp.from(EPOCH.plusDays(dateValue)
				.plusNanos(timeValue * 100)
				.toInstant(ZoneOffset.UTC)));
	}
}

Also, here is an excellent video by Aleksey Shipilev (Author of JMH and a major player in Java performance) about microbenchmark pitfalls.
https://www.youtube.com/watch?v=VaWgOCDBxYw

@cogman - Thanks for the tips. Clearly the Java compiler and JRE are a lot more clever than I gave them credit for.
Hoping to provide more meaningful input to this discussion, I tried to set up a JMH project in Eclipse using jmh-java-benchmark-archetype version 1.21. Unfortunately, while the project itself looks convincing it just doesn't work. If I do (the Eclipse equivalent of) mvn clean package I get a jar file named "datetime2-unpack-jmh-0.0.1-SNAPSHOT-shaded.jar" but when I try to run it I get
no main manifest attribute, in datetime2-unpack-jmh-0.0.1-SNAPSHOT.jar

Based on this Stack Overflow answer if I hack pom.xml to change ...
<plugin>
    <artifactId>maven-jar-plugin</artifactId>
    <version>2.4</version>
</plugin>

... to ...
<plugin>
    <artifactId>maven-jar-plugin</artifactId>
    <version>2.4</version>
    <configuration>
        <archive>
            <manifest>
                <addClasspath>true</addClasspath>
                <classpathPrefix>lib/</classpathPrefix>
                <mainClass>org.sample.MyBenchmark</mainClass>
            </manifest>
        </archive>
    </configuration>
</plugin>

(or variations on that theme) I consistently get
Error: A JNI error has occurred, please check your installation and try again
Exception in thread "main" java.lang.NoClassDefFoundError: org/openjdk/jmh/infra/Blackhole
        at java.lang.Class.getDeclaredMethods0(Native Method)
        at java.lang.Class.privateGetDeclaredMethods(Unknown Source)
        at java.lang.Class.privateGetMethodRecursive(Unknown Source)
        at java.lang.Class.getMethod0(Unknown Source)
        at java.lang.Class.getMethod(Unknown Source)
        at sun.launcher.LauncherHelper.validateMainClass(Unknown Source)
        at sun.launcher.LauncherHelper.checkAndLoadMain(Unknown Source)
Caused by: java.lang.ClassNotFoundException: org.openjdk.jmh.infra.Blackhole
        at java.net.URLClassLoader.findClass(Unknown Source)
        at java.lang.ClassLoader.loadClass(Unknown Source)
        at sun.misc.Launcher$AppClassLoader.loadClass(Unknown Source)
        at java.lang.ClassLoader.loadClass(Unknown Source)
        ... 7 more

Oh well. I tried.

@gordthompson This is maybe getting off topic.  But the problem is that "org.sample.MyBenchmark" doesn't contain an actual main static method.  In order to make this run, you need to hit the JMH main class. ( org.openjdk.jmh.Main )  It will do the scanning to find the benchmark classes for you.  You can either do that by adding a manifest or by directing java to run that class directly (
It has been a while since I've set this up, but attached is the entirety of my pom file for running jmh.
Few key points, it pulls out jar signing, because it is making an uberjar. Instead of using the jar plugin to write the manifest it is doing that as part of the shading plugin.
pom.xml.gz
https://stackoverflow.com/questions/5474666/how-to-run-a-class-from-jar-which-is-not-the-main-class-in-its-manifest-file

@cogman - Thanks again. The build was producing two separate jar files and I was simply trying to run the wrong one. (D'oh!)
Running your latest tests on my very-old-and-slow test box yielded
Benchmark                                         Mode  Cnt       Score       Error  Units
MyBenchmark.test_read_local_date_new             thrpt   25  210629.896 ±   726.327  ops/s
MyBenchmark.test_read_local_date_reuse_calendar  thrpt   25   95127.109 ± 12725.871  ops/s

If I'm reading that right, UTC_Timestamp via LocalDateTime is actually more like twice as fast as UTC_Timestamp via GregorianCalendar on my test box. Very different from my previous naïve attempt at benchmarking.
Still, it's not really a fair fight. The GregorianCalendar approach has to deal with the baggage of a Calendar object while the LocalDateTime approach just uses a fixed offset (UTC+0) and avoids the whole "messy rest-of-the-world time zone" thing. Since the code needs to be able to produce a Timestamp in any time zone (not just UTC) a "better" comparison would be to create the Timestamp in the JVM default time zone (which for me happens to be America/Denver) ...
package org.sample;

import org.openjdk.jmh.annotations.Benchmark;
import org.openjdk.jmh.annotations.Scope;
import org.openjdk.jmh.annotations.State;
import org.openjdk.jmh.infra.Blackhole;

import java.sql.Timestamp;
import java.time.LocalDateTime;
import java.util.*;

@State(Scope.Thread)
public class MyBenchmark {
	private static long dateValue = 737240L;
	private static long timeValue = 432000000000L;
	private static LocalDateTime EPOCH = LocalDateTime.of(1, 1, 1, 12, 0);

	private GregorianCalendar cal = new GregorianCalendar();  // default JVM time zone
	private java.util.Date foreverAgo = new java.util.Date(Long.MIN_VALUE);

	@Benchmark
	public void test_read_local_date_reuse_calendar(Blackhole bh) {
		bh.consume(makeReusingCalendar());
	}

	private Timestamp makeReusingCalendar() {

		cal.clear();
		cal.setGregorianChange(foreverAgo);  // proleptic
		cal.set(1, 0, 1, 0, 0, 0);  // 0001-01-01
		cal.add(GregorianCalendar.DAY_OF_YEAR, (int)dateValue);
		cal.add(GregorianCalendar.MILLISECOND, (int) (timeValue / 10000L));
		return new Timestamp(cal.getTimeInMillis());
	}

	@Benchmark
	public void test_read_local_date_new(Blackhole bh) {
		bh.consume(Timestamp.valueOf(EPOCH.plusDays(dateValue)
				.plusNanos(timeValue * 100)));
	}
}

... and the results I see are ...
Benchmark                                         Mode  Cnt       Score      Error  Units
MyBenchmark.test_read_local_date_new             thrpt   25  143919.857 ± 1135.624  ops/s
MyBenchmark.test_read_local_date_reuse_calendar  thrpt   25   74379.197 ±  375.586  ops/s

Again, if I'm reading things correctly, the LocalDateTime approach took a hit (from 211k ops/s to 144k ops/s) and so did the GregorianCalendar approach (from 95k ops/s to 74k ops/s) but the LocalDateTime approach still seems to be roughly twice as fast.

@ulvii I think us customers at least are all in agreement.  Switching over would generally be a good improvement for the driver both from a performance standpoint and from a ergonomic standpoint in the codebase.
If I get some time today I might dive in a bit and see if I can bang something out (or at least how big of an impact it would be).  Do you have any more questions for us?

Hi all,
I wrote a quick and dirty patch for benchmarking.
https://github.com/microsoft/mssql-jdbc/compare/dev...harawata:gh-1070?expand=1
I also created a JMH project, but am not sure if I did it right (I haven't gotten any consistent result probably because there are many other processes running on the same machine).

Hi everyone,
Thanks for all the effort. We will investigate the impact of the change for different JDBC/SQL types. In the meantime, please feel free to provide any other details that you think would speed up the process. 👍

Hi @gordthompson, @cogman, thanks for your input. I've recently starting looking into this issue, and I've set up my own JMH environment to measure the performance difference between relying on GregorianCalendar vs LocalDateTime. The results reflect the same findings we've discussed in this thread (ultimately, utilizing GregorianCalendar can be anywhere between 2~5 times slower than the LocalDateTime alternative, depending on the context). Therefore, I agree that we should move away from relying on GregorianCalendar in the driver, and I'm considering applying this change to the entire driver, wherever applicable.
However, I wanted to clarify one thing - it's the re-construction of the GregorianCalendar that is slow (setting the time values after calling .clear()), and simply retrieving data from the GregorianCalendar instance is fast (i.g. retrieve the TimeZone information). If the client has already provided a GregorianCalendar instance (along with the timezone information) to the driver, then I believe there shouldn't be a problem with using that instance to insert/select temporal objects.
This means that the driver only needs to replace instances in the driver where GregorianCalendar instances are being created (or being re-constructed), without having to modify most of the public/internal APIs. Is my understanding of the scope of the problem correct, or does anyone think that we should try to remove reliance on Calendar objects altogether (as much as we can) in the driver? (for example, perhaps try to replace the Calendar parameter in DDC::convertTemporalToObject with a TimeZone object, since it really only needs the TimeZone information in that method?) Thanks.

@peterbae  Creation is definitely the slowest part of the Calendar classes.
The big problem with the GregorianCalendar class is that it is HIGHLY thread-unsafe.  So any usage of it could potentially trigger thread safety/race condition problems that would be hard to track down.  That isn't to say that you couldn't use it in a thread safe manor, but that you'll need to inspect every method called on GregorianCalendar to evaluate whether or not it is mutating internal state (And I mean every method, for example, getTimeZone can mutate internal state).
You could add synchronization around it, but that will add overhead that will be hard to capture in a JMH benchmark (because the JVM does neat and terrible things with locks that will hide actual costs).
IMO, the safest thing to do is replace it outright rather than try to share instances of it across threads.  The new localdate stuff is all thread safe (it is all immutable objects).  Next safest and probably fastest (but potentially costly memory wise) would be thread local storage for the calendar object.

Hi @cogman, thanks for the input. I agree that we should use as little of GregorianCalendar as possible. I'd like to replace all instances of Calendar in the driver, but the JDBC specs state that the driver needs to provide public APIs that accept Calendar objects (mostly from getters/setters for temporal objects in prepared/callable statements).
Looking at how the driver actually uses the Calendar objects passed from the client, most of the time they're used for the timezone information. I'll look into extracting the necessary information from the Calendar object as soon as it's passed from the public API, while replacing as many instances of Calendar parameters with TimeZone parameters in internal methods. I'll also make sure that the driver never initializes any GregorianCalendar objects, either.
It'll take a while, but I'll update everyone once the initial PR/design has been made. Please let me know if you have any other input in the meantime.

Hi @gordthompson, @cogman , I've been working on converting all instances of Calendar to ZonedDateTime in the driver for a while. I have an initial implementation (https://github.com/peterbae/mssql-jdbc/tree/CalToZDT) that's still buggy when it comes to passing all of our internal testing. However, more importantly, I realized that this isn't feasible within the current implementation context of the driver.
There's a couple of problems with completely replacing Calendar with ZonedDateTime:

ZonedDateTime is the equivalent of a continuous Calendar object, but the driver was relying on a non-continuous calendar most of the time for the conversion. This means that for the dates before the Gregorian cutoff, the driver will need additional logic to replicate the original behavior of the non-continuous Calendar object if the driver were to completely migrate away from using Calendar objects for temporal data type conversion (I haven't been able to figure out how to do this as it seems the offset dates fluctuate depending on the actual year, but presumably this will be buggy and performance degrading).

From my testing, it seems like the Calendar object's timezone offset always follows the International Standard Time (IST) rules, whereas ZonedDateTime (and Instant) doesn't necessarily follow IST.
For example (my CalendarToZonedDateTime.testConvertFromTimestamp() method can demonstrate this), putting 1884-01-01 12:08:12.081 as the timestamp value won't fail the test, but changing that value to anytime before 1884 (the year when IST was adopted) fails, because the offset value goes from -8:00 to -8:12:28.

I think it's possible to solve the offset value problem by finding the expected offset value from Calendar, compare that to the offset value given by ZonedDateTime, then padding the difference to ZonedDateTime object (example: dtv.java#677), but this introduces major performance degradation. Here's the performance data I've measured before/after my changes, using my JMH framework:
Before my changes:
Benchmark                                     Mode  Cnt         Score        Error  Units
MyBenchmark.testExecuteCastAfterGregorian    thrpt   25      3158.933 ±     75.471  ops/s
MyBenchmark.testExecuteCastBeforeGregorian   thrpt   25      3157.668 ±     46.166  ops/s
MyBenchmark.testExecuteTableAfterGregorian   thrpt   25      1040.168 ±     29.996  ops/s
MyBenchmark.testExecuteTableBeforeGregorian  thrpt   25      1039.311 ±     34.864  ops/s
MyBenchmark.testGetDTOAfterGregorian         thrpt   25   2466225.354 ±  46751.922  ops/s
MyBenchmark.testGetDTOBeforeGregorian        thrpt   25   1134882.866 ±   7121.619  ops/s
MyBenchmark.testGetTimestampAfterGregorian   thrpt   25   2496906.544 ±  20329.848  ops/s
MyBenchmark.testGetTimestampBeforeGregorian  thrpt   25   1134950.349 ±   7908.037  ops/s
MyBenchmark.testSetDTO                       thrpt   25  24578113.784 ± 205272.912  ops/s
MyBenchmark.testSetTimestamp                 thrpt   25  24678204.585 ± 117714.068  ops/s

After my changes:
Benchmark                                     Mode  Cnt         Score        Error  Units
MyBenchmark.testExecuteCastAfterGregorian    thrpt   25      3110.902 ±     82.482  ops/s
MyBenchmark.testExecuteCastBeforeGregorian   thrpt   25      3159.163 ±     34.234  ops/s
MyBenchmark.testExecuteTableAfterGregorian   thrpt   25      1009.815 ±     43.995  ops/s
MyBenchmark.testExecuteTableBeforeGregorian  thrpt   25       985.471 ±     51.050  ops/s
MyBenchmark.testGetDTOAfterGregorian         thrpt   25    448032.106 ±   7980.397  ops/s
MyBenchmark.testGetDTOBeforeGregorian        thrpt   25    438564.948 ±  17780.253  ops/s
MyBenchmark.testGetTimestampAfterGregorian   thrpt   25    449824.718 ±   6852.955  ops/s
MyBenchmark.testGetTimestampBeforeGregorian  thrpt   25    453920.562 ±  22323.679  ops/s
MyBenchmark.testSetDTO                       thrpt   25  24503831.627 ± 214236.776  ops/s
MyBenchmark.testSetTimestamp                 thrpt   25  24520602.552 ± 174342.838  ops/s

The meaningful results are from the testGet... methods. They're slower after my changes because of the additional logic to compensate for the difference in offset values between Calendar's offset values and ZonedDateTime's offset values, even though we know that ZonedDateTime should be faster than Calendar normally.
Also, having to do this conversion from the offset data from Calendar means the driver isn't completely moving away from relying on Calendar objects anymore, so there's less reason to move forward with this change.
I'm not 100% certain if I'm doing everything correctly, but so far my findings show that replacing Calendar to ZonedDateTime in the driver is not reasonable. However, going back to the original
issue that @lavabear has created (and also the other issue #1088), I think we should be able to fix SQLServerResultSet.getObject(int, LocalDateTime) case so that we improve the performance / fix date cases before Gregorian cutoff without having to deal with the problems I mentioned above (to clarify, that means I will be fixing issue #1070 and #1088 without replacing Calendar with ZonedDateTime in most parts of the driver). Does that sound reasonable, or does anyone have any other suggestions? Thanks a lot for everyone's input on this issue.

1. ZonedDateTime is the equivalent of a continuous Calendar object, but the driver was relying on a non-continuous calendar most of the time for the conversion. This means that for the dates before the Gregorian cutoff, the driver will need additional logic to replicate the original behavior of the non-continuous Calendar object if the driver were to completely migrate away from using Calendar objects for temporal data type conversion (I haven't been able to figure out how to do this as it seems the offset dates fluctuate depending on the actual year, but presumably this will be buggy and performance degrading).

Are you sure the old behavior is required and intended rather than a bug? My understanding is SQL Server is proleptic and java.time is proleptic so there shouldn't be a problem. We had a similar issue in pgjdbc https://github.com/pgjdbc/pgjdbc/issues/1534, turned out the code was buggy and never worked. Now the driver simply returns the database values.

I agree with @marschall . When retrieving a datetime2 column value directly to LocalDateTime the LocalDateTime value should simply be the exact same value as stored in the table, even if it is for a "non-existent" date like 1582-10-10. The current implementation does not handle this correctly because it used a non-continuous Gregorian Calendar to interpret the Timestamp value as UTC. (My bad.)

Thanks @marschall @gordthompson, I agree with this assessment as well. I think that means we should be able to solve the first problem easily. Do we have a way to deal with the second problem? In the meantime, I'll be looking into fixing the original issues from #1070 and #1088.

From my testing, it seems like the Calendar object's timezone offset always follows the International Standard Time (IST) rules, whereas ZonedDateTime (and Instant) doesn't necessarily follow IST.
...
I think it's possible to solve the offset value problem by finding the expected offset value from Calendar, compare that to the offset value given by ZonedDateTime, then padding the difference to ZonedDateTime object

I would argue that it is not necessary to ensure that values unpacked to a Calendar object and values unpacked to a ZonedDateTime object should yield the same results because they are different things. As long as the behaviour for legacy date/time objects (Calendar, java.sql.Timestamp) objects does not change — for backward compatibility — the ZonedDateTime value can be whatever java.time thinks it should be.
In other words, if datetime2 values are always initially unpacked to LocalDateTime and then, if desired, converted to java.sql.Timestamp (via a user-supplied Calendar) or to java.time.Instant (via a user-specified time zone) then mssql-jdbc would be "playing by the rules" and any discrepancies between Calendar and ZonedDateTime values would be something for the Java team to explain.

I'm with @gordthompson on this one.
I think as far as bugs go, while there may be some cases where this causes issues, I can't imagine there are very many applications that care about the exact timestamp of a date from before 1884.  Doesn't really seem like something you'd run into all that often.
I know I'll probably eat these words though as I'm sure some legal document engine is critically dependent on the exact minute some 14th century document was signed.

Thanks everyone for your ideas. I agree that we don't necessarily have to make sure that Calendar and ZonedDateTime objects have to yield the same results. What I meant to convey was that ultimately there's little point in modifying the code inside DDC.convertTemporalToObject to replace all Calendar instances with ZonedDateTime instances, because due to the difference in how offsets are calculated before 1884, we still need to rely on the existing Calendar object. Also, having to re-adjust the ZonedDateTime object with the difference in offset value is costly (as shown in the JMH benchmark data I posted above) since it seems like we will need to return java.sql.Timestamp value pretty often from the method. As for @cogman's point regarding the dates before 1884 not being used often, our test suites contain data that goes before 1884 (also the other issue #1088 specifically deals with data before 1582), so we don't want the driver to retrieve incorrect data for the user.
To summarize, I'm suggesting that I should fix the original issue that was brought up by @lavabear instead of doing a complete overhaul of how we send/retrieve temporal data. If we really want to replace all instances of Calendar instances with ZonedDateTime in the driver (even though it seems like we will need to rely on Calendar API to some degree), then we should track it in a separate issue. As for how we will fix this issue regarding performance when calling rs.getObject(int, LocalDateTime.class), we could simply go with the original suggestion from @gordthompson because it looks like the Calendar object that gets created is only queried for its timezone value without any modifications, and it shouldn't pose any concurrency issues. Please let me know if there's a better solution that doesn't involve a global GregorianCalendar instance or making major changes to the driver's logic.

I don't think you should be looking to simply replace Calendar with ZonedDateTime in the existing code. I think you should be looking to eliminate Calendar wherever possible. That would mean unpacking datetime2 columns directly to LocalDateTime, and unpacking datetimeoffset columns directly to OffsetDateTime. Calendar may still have to be used for things like getTimestamp("colname", Calendar), but in those cases the user would be supplying their own Calendar anyway.
The following POC code illustrates what I mean. It shows that java.time can directly convert TDS bytes to LocalDateTime for the different datetime2(precision) values, at least for a reasonably current date:
package com.example.sandbox;

import java.nio.ByteBuffer;
import java.nio.ByteOrder;
import java.time.LocalDateTime;
import java.time.ZoneId;

public class SandboxMain {

    public static void main(String[] args) throws Throwable {
        for (int precision = 0; precision <= 7; ++precision) {
            decodeDatetime2ForPrecision(precision);
        }
    }

    private static void decodeDatetime2ForPrecision(int precision) {
        byte[] tdsByteArray = getDatetime2BytesFromTdsPayload(precision);

        // decode datetime2 bytes as per TDS specification
        //
        // https://docs.microsoft.com/en-us/openspecs/windows_protocols/ms-tds/786f5b8a-f87d-4980-9070-b9b7274c681d

        Integer multiplier = null;  // for conversion to nanoseconds
        switch (precision) {
            case 0: multiplier = 1_000_000_000; break;
            case 1: multiplier = 100_000_000; break;
            case 2: multiplier = 10_000_000; break;
            case 3: multiplier = 1_000_000; break;
            case 4: multiplier = 100_000; break;
            case 5: multiplier = 10_000; break;
            case 6: multiplier = 1_000; break;
            case 7: multiplier = 100; break;
        }

        System.out.printf("Precision = %d%n", precision);

        int timeLength = tdsByteArray.length - 3;  // DATE part is always 3 bytes
        ByteBuffer bb = ByteBuffer.allocate(12);
        bb.put(tdsByteArray, 0, timeLength);  // TIME part
        bb.position(8);
        bb.put(tdsByteArray, timeLength, 3);  // DATE part
        bb.position(0);
        bb.order(ByteOrder.nativeOrder());

        long fractionalSeconds = bb.getLong();
        System.out.printf("  TIME part: %d%n", fractionalSeconds);

        int days = bb.getInt();
        System.out.printf("  DATE part: %d%n", days);

        LocalDateTime ldt = LocalDateTime.of(1, 1, 1, 0, 0)
                .plusDays(days)
                .plusNanos(fractionalSeconds * multiplier);
        System.out.printf("  LocalDateTime: %s%n", ldt.toString());
    }

    private static byte[] getDatetime2BytesFromTdsPayload(int precision) {
        // --- Test Data ---
        //
        // test case: SELECT CAST('2017-01-01 01:23:45.1' AS DATETIME2(precision))
        //
        byte[] tdsBytes = null;  // actual TDS bytes from server (via Wireshark)
        switch (precision) {
            case 0:
                tdsBytes = new byte[] { (byte) 0xa1, (byte) 0x13, (byte) 0x00,
                        (byte) 0x49, (byte) 0x3c, (byte) 0x0b };
                break;
            case 1:
                tdsBytes = new byte[] { (byte) 0x4b, (byte) 0xc4, (byte) 0x00,
                        (byte) 0x49, (byte) 0x3c, (byte) 0x0b };
                break;
            case 2:
                tdsBytes = new byte[] { (byte) 0xee, (byte) 0xaa, (byte) 0x07,
                        (byte) 0x49, (byte) 0x3c, (byte) 0x0b };
                break;
            case 3:
                tdsBytes = new byte[] { (byte) 0x4c, (byte) 0xad, (byte) 0x4c, (byte) 0x00,
                        (byte) 0x49, (byte) 0x3c, (byte) 0x0b };
                break;
            case 4:
                tdsBytes = new byte[] { (byte) 0xf8, (byte) 0xc4, (byte) 0xfe, (byte) 0x02,
                        (byte) 0x49, (byte) 0x3c, (byte) 0x0b };
                break;
            case 5:
                tdsBytes = new byte[] { (byte) 0xb0, (byte) 0xb1, (byte) 0xf3, (byte) 0x1d, (byte) 0x00,
                        (byte) 0x49, (byte) 0x3c, (byte) 0x0b };
                break;
            case 6:
                tdsBytes = new byte[] { (byte) 0xe0, (byte) 0xf0, (byte) 0x84, (byte) 0x2b, (byte) 0x01,
                        (byte) 0x49, (byte) 0x3c, (byte) 0x0b };
                break;
            case 7:
                tdsBytes = new byte[] { (byte) 0xc0, (byte) 0x68, (byte) 0x31, (byte) 0xb3, (byte) 0x0b,
                        (byte) 0x49, (byte) 0x3c, (byte) 0x0b };
                break;
        }
        return tdsBytes;
    }
}

Console output:
Precision = 0
  TIME part: 5025
  DATE part: 736329
  LocalDateTime: 2017-01-01T01:23:45
Precision = 1
  TIME part: 50251
  DATE part: 736329
  LocalDateTime: 2017-01-01T01:23:45.100
Precision = 2
  TIME part: 502510
  DATE part: 736329
  LocalDateTime: 2017-01-01T01:23:45.100
Precision = 3
  TIME part: 5025100
  DATE part: 736329
  LocalDateTime: 2017-01-01T01:23:45.100
Precision = 4
  TIME part: 50251000
  DATE part: 736329
  LocalDateTime: 2017-01-01T01:23:45.100
Precision = 5
  TIME part: 502510000
  DATE part: 736329
  LocalDateTime: 2017-01-01T01:23:45.100
Precision = 6
  TIME part: 5025100000
  DATE part: 736329
  LocalDateTime: 2017-01-01T01:23:45.100
Precision = 7
  TIME part: 50251000000
  DATE part: 736329
  LocalDateTime: 2017-01-01T01:23:45.100

BTW, I just did another test using a datetime2 value from the year 1066 and that unpacked correctly as well.

Hi all,
I've made PR #1200 that addresses this issue. I was able to eliminate the usage of Calendar when it comes to most getter cases, except for the cases where the user provides their own Calendar instance (this automatically implies the new code doesn't affect DateTimeOffset scenarios, since DateTimeOffset is always associated with a UTC timezone Calendar). I've also optimized the getter scenarios for LocalDateTime/LocalDate/LocalTime.
I've updated my JMH test suite to cover all of the data conversion scenarios. The results can be found here (dev) and here (with current change). The data shows that we see a significant performance improvement whenever there is no Calendar object involved.
The reason why I decided to use the old code path when a custom Calendar object is involved is because I couldn't quite get all of the edge cases correct without using the Calendar that was provided by the user (whenever it is provided), and my implementation wasn't very optimized to a point where it was performing worse compared to simply relying on the old code path. Please let me know if there's a way to completely eliminate the use of Calendar object even when the user has provided their own Calendar instance.
I'm looking to see if I can apply the same changes to the setter scenarios so that we can optimize the performance to both getter/setter scenarios (and ideally make it into the next preview). In the meantime, if anyone has any suggestions/comments, please let me know.

8.1.1 Preview Release is out, which contains this fix. Please give the preview a try, and let me know of any further possible improvements that could be made. Otherwise, I will close this issue in due time.

authors: cogman gordthompson harawata lavabear marschall peterbae rene-ye ulvii

repo: microsoft/mssql-jdbc


text:
Add support for a multi-release jar

When upgrading from java8 to Java11+ one has to treat the mssql-jdbc dependency different to other jar dependencies, since as part of the java upgrade you will have to also upgrade the application itself to use the newer jar dependency.
Java supports multi-release jars https://openjdk.org/jeps/238, i.e. jars that contain classes for both java8 and java11+, why can this project not be shipped with a single jar that is backwards compatible with older java versions?

why can this project not be shipped with a single jar that is backwards compatible with older java versions?

That's not something we've explored yet, but does sound useful to do. I'll bring this up to the team at the next triage. Marking this as a potential enhancement for now.

source: gharchive/issue

authors: mbbergma tkyc

license: MIT

license_type: permissive

license_source: github-api


text:
Current DE CU Releases not available

Hi,
I'm missing the docker images for:
NAV 2018 CU17 DE
NAV 2017 CU30 DE
NAV 2016 CU43 DE
When will they be available?

@freddydk some Info on this?

mcr.microsoft.com/dynamicsnav:2018-cu17-de
I will write up a blog post on new location of images (have mentioned it a few times, but not final)

Ah, perfect. Was always trying "mcr.microsoft.com/dynamics-nav". Stupid me 😅

repo: microsoft/nav-docker


text:
Run-TestsInBcContainer without specifying extensionId does not work

I'm trying to run all Microsoft Tests together with my Custom Tests in a DevOps Pipeline using Run-AlPipeline.
I'm not passing appfolders but only one app in the testfolders. In the test app I have an Install codeunit that creates the Test Suite called "DEFAULT" with range of the objects '50000..'.
I overwritten RunTestsInBcContainer of Run-AlPipeline to remove from the parameters the extensionId.
Here the result
Running tests for App 96c1fc3a-3d7b-40a7-ace4-1e232d49bd05 in C:\1\_work\9\s\TestApp

Connecting to [http://localhost:80/BC/cs?tenant=default](http://localhost/BC/cs?tenant=default)
Run-Tests, open page 130455
Show form 0001fd97-0000-0001-0c00-0000836bd2d2
Title: Command Line Test Tool Edit - Command Line Test Tool - DEFAULT
Disabling Test CodeunitName:Method
Using repeater based test-runner
Offset: 0
Offset: 0
Index:  0, Offset: 0, Count:  2
Row - lineType = 1, run = Yes, CodeunitId = 0, codeunitName = '', name = ''
Found Row - index = 1, rowIndex = 0/2, lineType = 1, run = Yes, CodeunitId = 0, codeunitName = '', name = ''

My feeling is that Run-TestsInBcContainer without specifying extensionId stopped working. According to the example and documentation it should run all tests.
I've tested also with the preview version of BCContainerHelper but same result.

At this point https://github.com/microsoft/navcontainerhelper/blob/09f2cfd6ef3bd0c925915e8ea169f34646e22a89/AppHandling/PsTestFunctions.ps1#L546
The default test page is 130455 then in th next line set the extensionId even if there is not extensionId
Set-ExtensionId -ExtensionId $extensionId -Form $form -ClientContext $clientContext -debugMode:$debugMode

Will have a look.
I would recommend NOT using testsuites and special test runners - but of course they should still work.
At some point in time, we will be refactoring test runners and allow running tests through the Dev Endpoint from VS Code.
This will probably not be using AL Test Runners and Test Suites.

At this point
https://github.com/microsoft/navcontainerhelper/blob/09f2cfd6ef3bd0c925915e8ea169f34646e22a89/AppHandling/PsTestFunctions.ps1#L546
The default test page is 130455 then in th next line set the extensionId even if there is not extensionId
Set-ExtensionId -ExtensionId $extensionId -Form $form -ClientContext $clientContext -debugMode:$debugMode

Correct, but the Set-ExtensionId function returns immediately if extensionId is empty
I know that ALOps still supports the old way of running tests, so it should work.
However, if you can create a very small test app with a single test function and send it my way (including how you run it), then I will see where the error is.

Hi,
I've attached a test app that includes a custom test plus an install codeunit that creates the testsuite with all tests, mine plus the the Microsoft Standard tests.
TestApp.zip
I tried to run in different ways:

Customized Run-AlPipeline to owerwrite RunTestsInBcContainer and remove the extensionId

$RunTestsInBcContainer = {
    Param([Hashtable]$parameters)
    $parameters.Remove('extensionId')
    $parameters
    Run-TestsInBcContainer @parameters
}

Create the container, deply the extension and then run from PowerShell the command
Run-TestsInBcContainer -containerName $containerName -credential $credential -detailed -debug -debugMode

In both cases I'm getting the same result.

Hi Freddy,
I'm sorry but I don't understand from your message how you identified thta I'm using CAL Test Runner. It's not my intention to use functionalities that will disappear. Can you please add more details on where I should apply the chnages?
you should really create the test suite in the AL Test Runner objects
Please clarify this sentence

CAL Test Suite
and the Run-Tests will execute tests from the AL Test Suite (I think)

It has been many years since I looked at this last time - I don't use test suites anymore - but I am pretty certain that I remember that there are two different sets.

Thank you, I fixed the code and made it working.

authors: eclipses freddydk

repo: microsoft/navcontainerhelper


text:
ERROR: Kill trial job failed

I want to kill a trial job,but always faild.
The alg run in windows local.and   use "nnictl trial ls" get the trial message as fllows:
[
{
"hyperParameters":[
"{"parameter_source":"algorithm","parameter_id":0,"parameter_index":0,"parameters":{"learning_rate":0.001,"conv_size":2,"dropout_rate":0.7907991690451954,"batch_size":32,"hidden_size":124}}"
],
"id":"af4FB",
"logPath":"file://localhost:C:\Users\someone\nni\experiments\QM3Nnbkf\trials\af4FB",
"sequenceId":0,
"startTime":"2019/07/04 06:17:05",
"status":"RUNNING"
}
]
Now,I want to kill it . use code as  :  nnictl trial kill -T af4FB    or  nnictl trial kill -T "af4FB"
but it failed.
please give me some advises.

@cvJie - which NNI version are you using? any error message when it fails?

repo: microsoft/nni


text:
Add minimize mode for hyper-parameter graph

Short summary about the issue/question:
I set optimize_mode: minimize in my config.yml file. but hyper-parameter graph still choose maximize data for top trials.

Brief what process you are following:

How to reproduce it:

nni Environment:

nni version: v1.5
nni mode(local|pai|remote): local
OS:
python version:
is conda or virtualenv used?:
is running in docker?:

need to update document(yes/no):
Anything else we need to know:
Log message:

nnimanager.log and dispatcher.log :

nnictl stdout and stderr :

had fixed in PR #2433.


text:
no need to learn mask when requires_grad is False

some networks may set requires_grad to false at some layers.
When requires_grad of weight or bias  is set to false, we don't need to mask them out.

I think whether the parameter needs a gradient or not will not affect whether the value should be masked. If this parameter has a corresponding mask tensor, then it should be masked.

requires_grad controls weight update, masks are configured through op_names and op_type.
We do not assume that requires_grad can affect masks configuration.

agree

source: gharchive/pull-request

authors: chicm-ms twmht zheng-ningxin


text:
Handle nulls better in scaleset shutdown

Summary of the Pull Request
What is this about?
PR Checklist

[ ] Applies to work item: #xxx
[ ] CLA signed. If not, go over here and sign the CLI.
[ ] Tests added/passed
[ ] Requires documentation to be updated
[ ] I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #xxx

Info on Pull Request
What does this include?
Validation Steps Performed
How does someone test & validate?

Codecov Report

Merging #2538 (f744fb3) into main (e83a18b) will decrease coverage by 5.23%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main    #2538      +/-   ##
==========================================
- Coverage   29.84%   24.61%   -5.24%
==========================================
  Files         288      119     -169
  Lines       35673    12400   -23273
==========================================
- Hits        10648     3052    -7596
+ Misses      25025     9348   -15677

Impacted Files
Coverage Δ

...ervice/ApiService/onefuzzlib/ScalesetOperations.cs
2.65% <0.00%> (-0.01%)
:arrow_down:

src/agent/onefuzz-agent/src/setup/double.rs

src/agent/dynamic-library/src/linux.rs

src/agent/onefuzz-task/src/managed/cmd.rs

src/agent/coverage/src/test.rs

src/agent/coverage/src/intel.rs

src/agent/onefuzz/src/heartbeat.rs

...ent/onefuzz-task/src/local/libfuzzer_regression.rs

src/agent/onefuzz-task/src/local/libfuzzer_fuzz.rs

src/agent/onefuzz-agent/src/commands.rs

... and 160 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

authors: codecov-commenter tevoinea

repo: microsoft/onefuzz


text:
How to get DOM element by JavaScript GetElementsByClassName in Playwright-Sharp?

Hello:
I am rather new to Playwright-Sharp, I need to run some very simple JavaScript in Playwright-Sharp.
document.getElementsByClassName('class1')[0].click();
sleep 1 second;
document.getElementsByClassName('class2')[0].value = 1;
then move focus from document.getElementsByClassName('class2')[0] to other DOM elements.
For sleeping 1 second, I can use some JavaScript Promise,
but how to move focus after setting an input element value, I don’t know yet.
Please show me how I can do this by Playwright-Sharp (version 0.192.0)
I searched around, I only see something like for select DOM element:
page.WaitForSelectorAsync('#id1');
But this code doesn’t work in my case, as there is no ID# in the whole web page.  I have to use getElementsByClassName.
By the way, there is no example code on how to set value for an input element.
Please advise,

Class names can be used in the CSS selector: https://playwright.dev/dotnet/docs/selectors
Input can be set as follows:
await page.EvalOnSelectorAsync<string>("class-name", "(i, value) => i.value = value", "VALUE");

authors: pavelfeldman zydjohnHotmail

repo: microsoft/playwright-sharp


text:
[BUG]: broken icon links in e-mails from Nurture Components solution

Describe the bug
E-mail templates sent from Nurture Components solution contain broken link to Power Automate logo in
the top ribbon (http://powerplatformconference.com/wp-content/uploads/2019/05/FlowIcon256x256.png).
Our client asked us to fix this somehow, but then it would be potentially overwritten by the upgrade and we would need to handle it again and again. That's why we're reporting this. We know it's just a cosmetic issue, but it doesn't make a good impression.
Talking about the logos, the Power Apps one is older version (https://higherlogicdownload.s3.amazonaws.com/PBIUSERGROUP/0dd17701-ca4a-49ad-9e12-8a3458d663ba/UploadedImages/World_Tour/2019/PowerApps.png), but at least it's not displayed like the missing one:

Component (please tell us which flow or app you are experiencing issues with):

Solution Nurture
Version 1.23
App or Flow Training In A Day | Reminder 3 days prior event
Training In A Day | Registration Confirmation
Training In A Day | Feedback Reminder
Admin | Welcome Email

To Reproduce
I believe not needed, it's sufficient to open the flow, find the Outlook action and check the image links.
Expected behavior
Ribbon either containing no Power Apps/Automate logo, or correctly linked.

Hello. Did you fill in the logo field during setup? configure the coe settings table

Hello. Did you fill in the logo field during setup? configure the coe settings table

Sorry from above perhaps I've mis-read. Is your concern about the PowerApps logo or the missing customer logo?
If missing customer logo, then please be sure you've set up the logo field.

Sorry from above perhaps I've mis-read. Is your concern about the PowerApps logo or the missing customer logo?
If missing customer logo, then please be sure you've set up the logo field.

Hi :) no, I am not talking about the customer logo, but the power apps and power automate logos in nurturing e-mails. Marked at the screenshot:

Hi :) no, I am not talking about the customer logo, but the power apps and power automate logos in nurturing e-mails. Marked at the screenshot:

Got it, thanks so much!

Got it, thanks so much!

Our customer also wanted us to change the ribbon color from purple to dark blue (still talking about the screenshot above)

I have done that in CSS used in the environment variable used for e-mail headers. But then the purple powerapps logo looked really... not good (purple logo on a dark blue ribbon) 😃 so I this way I learned that the logo actually is purple, not grayscale transparent 😄 but that's just FYI, that such requests also happen

Our customer also wanted us to change the ribbon color from purple to dark blue (still talking about the screenshot above)

I have done that in CSS used in the environment variable used for e-mail headers. But then the purple powerapps logo looked really... not good (purple logo on a dark blue ribbon) 😃 so I this way I learned that the logo actually is purple, not grayscale transparent 😄 but that's just FYI, that such requests also happen

We have removed these from the email locally and the fix will ship with our next monthly update.
Will hold to let you know when that occurs.
Thanks for using CoE!

We have removed these from the email locally and the fix will ship with our next monthly update.
Will hold to let you know when that occurs.
Thanks for using CoE!

thank you! 👍

thank you! 👍

Hello. This change in the release today.
Please be sure to remove any unmanaged layers before updating as described in installing upates.
And thanks for helping make CoE great!

authors: JeneferM-MSFT poppynka

repo: microsoft/powerapps-tools


text:
[BUG] Execution failures due to import issues when using additional_includes

Describe the bug
Bug description is based on the repro here.
When executing two flows in succession (and using additional_includes), the second flow can be made to fail depending on whether the first flow executed first or did not execute at all, even though there is no dependency (e.g. via the run argument) between the two flows.
The error message is related to import failures:
promptflow._core.tool_meta_generator.PythonLoadError: Failed to load python module from file '/tmp/tmptox7thdp/hello.py': (ModuleNotFoundError) No module named 'common.parent_b'

I would expect that the flows succeed or fail independently of each other, but that there should be no interference between the two flows.
How To Reproduce the bug
Clone the repro repository

Run python run_bug.py to observe the import failure
Run python run_no_bug.py to observe that when running the second flow alone, there is no import failure.

Expected behavior
The eval flow should consistently succeed or fail independently of whether the main flow ran.
Running Information(please complete the following information):

Promptflow promptflow==1.5.0
Operating System: WSL2
Python 3.9.18

Hi @bastbu , thanks for reporting this. The root cause for this issue is we used user's process to resolve tool modules and different flows may affect each other due to module cache. We'll change pf.run to execute in separate processes to avoid the issue. I'll update here when the fix releases. A workaround before that would be run different pf.run in different processes like this:
    client = PFClient()

    p1 = Process(target=run_flow, args=(client, "main", "main/data.jsonl"))
    p2 = Process(target=run_flow, args=(client, "eval", "eval/data.jsonl"))

    p1.start()
    p2.start()

    p1.join()
    p2.join()

Hi @D-W-, this issue was closed by the bot but I didn't verify whether this been addressed yet. Could you check, please?

Reopened the issue to track the fix, @bastbu , does the workaround above work for you? We hasn't change to load tools in subprocess in prompt flow side due to performance concern.

authors: D-W- bastbu

repo: microsoft/promptflow


text:
Support OpenAI-compatible APIs  for other endpoints [Feature Request]

Is your feature request related to a problem? Please describe.
For example I want to test LLaMa2 for AI application.
Describe the solution you'd like
I like promptflow treat  fastchat API which is OpenAI-compatible as a new endpoint.
Describe alternatives you've considered
Direct support huggingface model (auto its loading into a API endpoint) is perfect.
Additional context

Hi @whitelilis, we are about to launch a new tool named "Open Source LLM", which will accommodate Falcon or Llama2 in PromptFlow. This tool is set to be available next week. You can then refer to the tool documentation to utilize Llama2.
https://microsoft.github.io/promptflow/reference/tools-reference/open_source_llm_tool.html

authors: jiaochenlu whitelilis


text:
Ring tone not working

Describe the bug
ring tone block doesn't play any sound
To Reproduce
Steps to reproduce the behavior:

Go to https://makecode.com/_6chRpdAKpDup
See error

Expected behavior
ring tone should play sound
Desktop (please complete the following information):

OS: mac (and windows)
Browser chrome

@mmoskal It looks like you're making changes to mixer right now, so this might be a good one to look at real quick; it looks like it's just that ringTone passes 0ms as a time which is not handled in playTone and is ignored by addNote

fixed

repo: microsoft/pxt-arcade


text:
Import Error Message: "Import 'nose.tools' could not be resolved Pylance(reportingMissingImports)

Hi,
I am using VSCode for a python project. When I try "from nose.tools import assert_equal" to test my codes, the error message (above) populates.
After I install "pip install nose"  from cmd, I can get the correct output. However, the error message still persists.  Any insights?
Thank you.
Evan

Hi,
Have you set the interpreter path
i assume this would help
https://www.youtube.com/watch?v=x-BtaZDbQeo

Thanks for the report. Would you please check if the right virtual environment is being used?
The way to do that would be to:

If you have not selected a virtual environment, the status bar will show the global python interpreter.
Click on version number and choose the right environment from the command palette
If there is no environment offered, you will need to create an environment and then select that as the default environment
When you select the right virtual environment Pylance find the modules installed in that environment.

Here is a link to virtual environments: https://code.visualstudio.com/docs/python/environments
Please reopen this issue if this does not solve the problem.

authors: evanxuw judej sutr19

repo: microsoft/pylance-release


text:
Default diagnostic severity levels should be reconsidered

From Marketplace review:

Robert Buch
Some useful reports are disabled by default, even on the "strict" type-checking mode. I had to manually set most of these to "warning" or "information". Speaking of, all reports are set to "error" by default, which seems rather unreasonable. Is it really an "error" if you have an unused import? Is the world going to end if I have an unnecessary "isinstance()" call? Such reports should be marked as "warning" or "information".

I think all of these could be argued either way. 'Strict' means strict so you don't want any reports, even unused imports.
Transferring to a discussion.

authors: debonte rchiodo


text:
Pylance with a multi-root workspace

Discussed in https://github.com/microsoft/pylance-release/discussions/6303

Originally posted by splint33 August 27, 2024
Hi everyone, I'm kinda new to Pylance, sorry if my question is dumb.
I have a VSCode workspace, containing multiple folders. Each folder is a GIT repository.
My code-workspace is looking like that at the moment:
{
    "folders": [
        {
            "name": "root",
            "path": "."
        },
        {
            "name": "lib_1",
            "path": "C:\\AutomatedTests\\lib_1"
        },
        {
            "name": "lib_2",
            "path": "C:\\AutomatedTests\\lib_2"
        },
        {
            "name": "lib_3",
            "path": "C:\\AutomatedTests\\lib_3"
        }
    ],
    "settings": {
        "python.analysis.extraPaths": [
            "${workspaceFolder:root}/scripts",
            "${workspaceFolder:lib_1}",
            "${workspaceFolder:lib_2}",
            "${workspaceFolder:lib_3}"
        ]
}

When I edit lib1, I must restart the language server in order to be able to see changes in the root folder.
I understand that Pylance is indexing each folder individually at the start of the language server, and seems to use a cached version of the index.
Changes made into lib1 are reflected into lib1 immediately, but not on other workspace folders.
Is there a way to keep this structure and make it work with pylance ?
Thanks

we assumed extraPath is part of workspace but it could be full path to outside of workspace. in that case, we need to set up file watcher for it.

I am finding multiple issues here. tackling one by one.

when extra path is used to see repro each other in mono repo (multi root workspace) set up, we are not propagating text changes properly to all workspaces.
when extra path is point to external path outside of any workspaces, we are not doing file watches on those folders.
persisting indices for packages outside of builtin stubs has a race for multi root workspace and sometime fail to persist affecting perf
there are room for improvement on indices sharing on multi root workspace (we already do, but certain modules we re-index them just to drop them since those modules don't have any symbols in them)

Item 2 was an intentional design choice, but maybe it's worth revisiting. The extraPaths setting is intended to be used for project-relative paths only. It wouldn't make sense to use absolute paths here because such paths would be invalid for anyone else working on a project. That said, a project-relative path could point to a location above the workspace's root directory, so one could argue that there are legitimate use cases not covered by existing file watchers. Do we have evidence that this use case is something users want pylance to support?

We (Pylance) have already been supporting absolute paths (or relative paths outside of a root) in extra paths for multi-root workspace users. They can also use ${workspaceFolder:xxx} to point to other roots in the settings.json.
There are multiple ways users have been doing this, such as legacy editable installs, PYTHONPATH, etc., and extra paths are also one of the methods users have been utilizing.
They could also place all repositories under the same root and use other configurations (extra paths, execution environments, include/exclude, etc.) to set it up under one workspace.
However, we don't force them to use any particular method since each has its own pros and cons.
for example, if a user is the only one using it, multi root workspace that points to each repo with extra path with${workspaceFolder:xxx} in settings.json is the simplest way to do it.

This issue has been fixed in prerelease version 2024.9.100, which we've just released. You can find the changelog here: CHANGELOG.md

authors: StellaHuang95 erictraut heejaechang


text:
auto import not working

Environment data

Pylance version: v2024.8.1
OS and version: WSL
Python version (& distribution if applicable, e.g. Anaconda):  poetry 3.11.8

settings.json
{
    "terminal.integrated.defaultProfile.windows": "Git Bash",
    "terminal.integrated.persistentSessionReviveProcess": "never",
    "terminal.integrated.enablePersistentSessions": false,
    "files.eol": "\n",
    "[python]": {
        "diffEditor.ignoreTrimWhitespace": false,
        "editor.formatOnType": true,
        "editor.wordBasedSuggestions": "off",
        "editor.formatOnSave": true,
        "editor.defaultFormatter": "ms-python.black-formatter",
        "editor.codeActionsOnSave": {
            "source.organizeImports": "explicit"
        }
    },
    "python.analysis.packageIndexDepths": [
        {
            "name": "sqlalchemy",
            "depth": 3,
            "includeAllSymbols": true,
        },
    ],
    "python.analysis.indexing": true,
    "python.analysis.autoImportCompletions": true,
    "python.analysis.autoSearchPaths": true,
    "python.analysis.importFormat": "absolute",
    "python.analysis.inlayHints.variableTypes": false,
    "python.analysis.inlayHints.functionReturnTypes": false,
    "python.testing.autoTestDiscoverOnSaveEnabled": true,
    "workbench.colorTheme": "Default High Contrast",
    "search.exclude": {
        ".idea": true,
        // ".venv": true,
        "poetry.lock": true
    },
    "editor.rulers": [
        120
    ],
    // formatter
    "pylint.args": [
        "--max-line-length=120",
        "--disable=C0114, C0115, C0116, W0105, C0200, E1101, E1121, C0103, W0621, W1203",
    ],
    "mypy-type-checker.args": [
        "--allow_redefinition"
    ],
    "black-formatter.importStrategy": "fromEnvironment",
    "black-formatter.args": [
        "--line-length=120"
    ],
    "notebook.formatOnCellExecution": true,
    // "flake8.importStrategy": "fromEnvironment",
    // "flake8.args": [
    //     "--config-toml=pyproject.toml"
    // ],
    "python.terminal.activateEnvInCurrentTerminal": true,
    "remote.autoForwardPortsSource": "hybrid",
    "workbench.editor.editorActionsLocation": "titleBar",
    "isort.importStrategy": "fromEnvironment",
    "isort.args": [
        "--profile",
        "black"
    ],
    "editor.defaultFormatter": "ms-python.black-formatter",
    "update.mode": "none",
    "extensions.autoUpdate": false,
    "editor.minimap.enabled": false,
    "explorer.compactFolders": false,
    "python.analysis.diagnosticMode": "workspace",
    "python.analysis.inlayHints.pytestParameters": true,
    "notebook.output.textLineLimit": 100,
    "files.autoSave": "onFocusChange",
    "debugpy.debugJustMyCode": false,
    "editor.formatOnSave": true,
    "notebook.formatOnSave.enabled": true,
    "[markdown]": {
        "diffEditor.ignoreTrimWhitespace": true
    },
    "jupyter.askForKernelRestart": false,
    "jupyter.debugJustMyCode": false,
    "[jsonc]": {
        "editor.defaultFormatter": "vscode.json-language-features"
    },
    "editor.stickyScroll.enabled": false,
    "files.insertFinalNewline": true,
}

what symbol are you looking for?

The variable in the other module - uuid_pk. It's not working because of this code here:
https://github.com/microsoft/pyright/blob/49235d38ba73200c014b1ae5b2617f1d67321352/packages/pyright-internal/src/languageService/autoImporter.ts#L467
Which looks like we only include 'CONSTANTS' as imports for variables.

this is by design then. auto import only suggests variables in __all__ unless it is from stub file.

But why do we not include other variables? I couldn't find anything that indicates these are considered 'private'. Private is only for variables with _ or __.

we already tried to include locals and back off since it includes too much stuff. python doesn't have accessibility mechanism and nobody uses _ for variables to indicates it is private at the top level. see any of our repro steps usual put in the issue. none of them are supposed to be public but none of them uses _.
our escape patch is __all__ for regular python file. if the variable is intended to be used in other module, it should be explicitly added to __all__ otherwise, we won't expose it to add/auto import

one thing we have we didn't have before is new search import code action and add import ordering with MRU, so, one thing we could do is exposing variables in search code action and add import with lowest priority?
I believe auto import should be left as it is.

@olegkorshunov that said, where is this uuid_pk defined? it would help for us to understand the usage case?

@olegkorshunov that said, where is this uuid_pk defined? it would help for us to understand the usage case?
here my project structure
app/db/custom_types.py: uuid_pk

I trying it to auto import in wallet.py
- app
    - __init__.py
    - db
       - __init__.py
       - custom_types.py.py
       - wallet.py
       ...
     ...
...

the

authors: heejaechang olegkorshunov rchiodo


text:
unresolved-import after adding package to repository (Poetry/Venv)

I'm using poetry for managing the environment and dependencies.
The behavior is that: any new packages added to the project (poetry add <package>) after starting up VSCode are not found (unresolved_import). Running the code from the terminal inside VS Code or a detached terminal with the venv activated does not encounter any import errors.

Restarting VS Code entirely will resolve the problem after the language server re-initializes.
Poetry is set to instantiate the virtualenv in the project folder, so .vscode/settings.json has the setting "python.pythonPath": ".venv/bin/python". The environment selection according to the status bar is also correct.

I am using the Microsoft language server:
{
    "python.jediEnabled": false,
    "python.languageServer": "Microsoft",
}

Originally posted by @Kyitrai in https://github.com/microsoft/vscode-python/issues/11934#issuecomment-647727468

This is currently expected behavior; to detect installed packages we need to watch files, and it proved more problematic than useful. You can enable this behavior manually with a setting: https://github.com/microsoft/python-language-server/blob/master/TROUBLESHOOTING.md#completions-do-not-update-andor-import-is-still-being-reported-as-unresolved-after-pip-install

authors: Kyitrai jakebailey

repo: microsoft/python-language-server


text:
Use d_feat and len_seq instead of 6 and 60.

Description
Use d_feat and len_seq instead of 6 and 60.
Motivation and Context
When d_feat and len_seq changed in yaml file, adarnn model will fail.

@microsoft-github-policy-service agree [company="mind0fpy"]

@microsoft-github-policy-service agree

@microsoft-github-policy-service agree company="mind0fpy"

repo: microsoft/qlib


text:
Data Grid Control with its sort, selection, filter, search, pagination & other data grid functions

Summary
The feature request for react-native-xaml involves integrating WinUI Data Grid Control including sorting, selecting rows, filtering, searching, pagination, and other essential data grid functions. This enhancement aims to provide a robust and user-friendly data grid experience within React Native windows applications by leveraging the capabilities of WinUI Data Grid.
Refer: [https://learn.microsoft.com/en-us/windows/communitytoolkit/controls/datagrid]
Motivation
The motivation behind this feature request is to enhance the data presentation and manipulation capabilities in React Native applications. Data Grid Control offers powerful features for managing and displaying large datasets efficiently. By incorporating these features into react-native-xaml, developers can create more sophisticated and responsive data-driven applications with improved usability.
Refer: [https://learn.microsoft.com/en-us/windows/communitytoolkit/controls/datagrid]
Open Questions
No response

hi @iammustafatz, this repo is for exposing the built-in windows.ui.xaml and microsoft.ui.xaml controls, and provides a way to expose controls from custom libraries. You could use the tools in this repo/library to expose an existing winui data grid control, here are the instructions: https://github.com/microsoft/react-native-xaml/blob/main/TechnicalGuide.md#using-a-custom-usercontrol

Let us know if the steps are successful in having you expose the existing community toolkit data grid control. Our expectation is that this should work.

authors: asklar chrisglein iammustafatz

repo: microsoft/react-native-xaml


text:
[Feature Request] Restler should test presence of auth files as part of settings validation

💡 Idea
Restler should test for presence of auth module if module authentication is specified during settings validation and fail fast if it is unreachable/does not exist
Design Notes
No response

Closed with https://github.com/microsoft/restler-fuzzer/pull/698

repo: microsoft/restler-fuzzer


text:
Kubernetes Ingress Controller Ignores Port Set in Kind:Ingress

Describe the bug
The Kubernetes Ingress controller always tries to access services on port 80 even if the Ingress resource is set at a different port.
For example, when the following configuration for port 5011 is provided, the logs read that it is attempting to add the service on port 80.
apiVersion: v1
kind: Service
metadata:
  name: test-ui
  labels:
    app: test-ui
spec:
  type: ClusterIP
  ports:
    - port: 5011
      targetPort: 80
  selector:
    app: test-ui
---
kind: Ingress
apiVersion: networking.k8s.io/v1
metadata:
  name: test-ui-ingress
spec:
  rules:
  - http:
      paths:
      - path: /
        pathType: Prefix
        backend:
          service:
            name: test-ui
            port:
              number: 5011

[11:38:32 DBG] New client created for cluster 'test-ui:80'.
[11:38:32 DBG] Cluster 'test-ui:80' has been added.
[11:38:32 DBG] Cluster 'test-api:80' has been removed.
[11:38:45 INF] Received Heartbeat for null

To Reproduce
What steps can we follow to reproduce the issue?

Navigate to the tutorial at https://github.com/microsoft/reverse-proxy/blob/main/docs/docfx/articles/kubernetes-ingress.md
Find the backend.yaml file referenced in the tutorial and change the port: 80 field to port:5011
Find the  ingress-sample.yaml file referenced in the tutorial and change the number: 80 field to number:5011
Run through the tutorial, when you try to access localhost you will receive a 404 in the logs. You can revert back to 80 to get it to work

I have tested the configuration with Contour and nginx ingress controllers to confirm it is a valid configuration.
Got Exceptions? Include both the message and the stack trace
No Exceptions, found, logs included above.
Further technical details

Include the version of the packages you are using
Built docker images from latest branch as referenced in documentation

The platform (Linux/macOS/Windows)

Windows 10

Tested on Kind (Kubernetes in Docker) and Docker Desktop for cluster

I've starting looking into this one. Looks like I need to add Service resource monitoring.

authors: dpbevin michaelkacher

repo: microsoft/reverse-proxy


text:
Split up the ProxyInvoker into a pipeline model

#49 Experimenting with a pipeline model for proxy customization. This creates a single pipeline for all proxy routes. The modules in the pipeline examine the endpoint metadata for route specific configuration.
I was able to split up the ProxyInvoker into three decoupled middleware:

EndpointInitializerMiddleware: Gets the current healthy endpoints from the route backend and sets AvailableBackendEndpointsFeature.Endpoints.
LoadBalancingMiddleware: Gets the available endpoints, invokes the ILoadBalancer service to reduce the set to one, and replaces AvailableBackendEndpointsFeature.Endpoints.
ProxyInvokerMiddleware: Gets the available endpoint (should be singular after load balancing) and proxies to it.

Conceptually this allows us to insert middleware before or after load balancing to vary the behavior. For example we can now add a separate affinity middleware before load balancing. It would pull the affinized endpoint from the available list and replace the list with that single element. This will cause load balancing to naturally short circuit since there's only one option.
The other middleware to consider is for request transformations. It might be easier to transform parts of the the request like the url components using the HttpContext object model than HttpRequestMessage since HttpContext treats the url as decomposed fields.
Is this a viable approach? Does it fit with our design proposals?

Is this a viable approach? Does it fit with our design proposals?

I think so.


text:
Diff is not showing where the changes where

When running a diff on a large directory, the diff output does not give any hints on where to look for the new issue.
For example the below:
    error level: +0 -0 no changes
    warning level: +1 +0
      New issue "deadcode.DeadStores Value stored to 'error' is never read" (1 occurence)
    note level: +0 -0 no changes
    all levels: +1 +0

To figure out where it happened I did the following:
sarif csv -o sarif_org ~/ovs/tests/clang-analyzer-results/2023-06-27-165114-1160621-1
sarif csv -o sarif_new ~/ovs/tests/clang-analyzer-results/2023-06-27-165604-1179114-1

sort sarif_org/static_analysis_output.csv > sarif_org.txt
sort sarif_new/static_analysis_output.csv > sarif_new.txt
diff sarif_org.txt sarif_new.txt

But this is also showing line changes, etc. etc.

Thanks for fixing this!! It works like a charm ;)
error level: +0 -0 no changes
warning level: +0 +0
  Number of occurrences 40 -> 41 (+1) for issue "unix.Malloc Use of memory after it is freed"
    file:///home/echaudron/Documents/review/ovs_quick/lib/netdev.c:243
note level: +0 -0 no changes
all levels: +0 +0

repo: microsoft/sarif-tools


text:
Method not found: 'Void Azure.AI.OpenAI.ChatCompletion Options.set_ChoicesPerPrompt(System.Nullable`1)'

I'm using Version 0.17.230718.1-preview.
Following the example from here - Example 17
When calling the method GenerateMessageAsync on  AzureChatCompletion object, I get the above error message.  Below is my code

_chatRequestSettings.MaxTokens = maxTokens;  _chatHistory.AddMessage(AuthorRole.User, prompt);  string reply = await _chatGPT.GenerateMessageAsync(_chatHistory, _chatRequestSettings).ConfigureAwait(false);
To Reproduce
Steps to reproduce the behavior:

Update nuget package to the latest version
Follow the example 17 to generate messages
See error

Expected behavior
The response from Azure OpenAI is returned.
Platform

OS: Windows 11
IDE: VS 2022
Language: C#
Source: NuGet package version 0.17.230718.1-preview.

Additional context
I tried with ChatCompletionsAsync method too. Got the same error. Below is the exception detail -
System.AggregateException
HResult=0x80131500
Message=One or more errors occurred. (Method not found: 'Void Azure.AI.OpenAI.ChatCompletionsOptions.set_ChoicesPerPrompt(System.Nullable`1)'.)
Source=System.Private.CoreLib
StackTrace:
at System.Threading.Tasks.Task.ThrowIfExceptional(Boolean includeTaskCanceledExceptions)
at System.Threading.Tasks.Task.Wait(Int32 millisecondsTimeout, CancellationToken cancellationToken)
at System.Threading.Tasks.Task.Wait()
at AIArt.UI.Business.OpenAIManager.GenerateGeneralResponse(String flowText, String prompt) in C:\Kidambi\GitHub\nkidambi\AIArT\AIArt.UI.Business\OpenAIManager.cs:line 20
at AIArt.Wpf.UserControls.ChatControl.<SendButton_Click>d__7.MoveNext() in C:\Kidambi\GitHub\nkidambi\AIArT\AIArt.Wpf\UserControls\ChatControl.xaml.cs:line 43
This exception was originally thrown at this call stack:
[External Code]
Microsoft.SemanticKernel.AI.ChatCompletion.ChatCompletionExtensions.GenerateMessageAsync(Microsoft.SemanticKernel.AI.ChatCompletion.IChatCompletion, Microsoft.SemanticKernel.AI.ChatCompletion.ChatHistory, Microsoft.SemanticKernel.AI.ChatCompletion.ChatRequestSettings, System.Threading.CancellationToken) in ChatCompletionExtensions.cs
AIArt.UI.Business.SemanticKernelManager.GetChatGPTResponse(string, string, int) in SemanticKernelManager.cs
AIArt.UI.Business.OpenAIManager.GenerateGeneralResponse.AnonymousMethod__0() in OpenAIManager.cs
Inner Exception 1:
MissingMethodException: Method not found: 'Void Azure.AI.OpenAI.ChatCompletionsOptions.set_ChoicesPerPrompt(System.Nullable`1)'.

Got the answer from Discord.
If referencing the Azure.AI.OpenAI package in the same project, beta 6 has a breaking change that’s incompatible with the current version of SK. Either downgrade to beta 5 or wait for the next SK release which updated to beta 6.

repo: microsoft/semantic-kernel


text:
.Net: Option to control temperature to stop Handlebars planner hallucinating functions.

I'm new to SK in the last few weeks and have only used the later versions (e.g. 1.2.0 and 1.0.0.-beta8).  I'm trying the Handlebars planner and finding that it is hallucinating some basic functions (e.g. "Example-Random") and in this example "length":
{{set "tables" (myCode-GetTablesInDatabase)}}

{{!-- Step 2: Choose the Right Helpers --}}
{{set "chosenTable" (range 0 (length tables) 1)}}
{{set "variables" (myCode-GetVariablesFromTable (lookup tables chosenTable))}}

It would be useful to have a way of turning down the temperature with the HandleBars planner.
This is the exception that I got:
An unhandled exception of type 'Microsoft.SemanticKernel.KernelException' occurred in System.Private.CoreLib.dll: '[HallucinatedHelpers] The plan references hallucinated helpers: Helper 'length''
 Inner exceptions found, see $exception in variables window for more details.
 Innermost exception      HandlebarsDotNet.HandlebarsRuntimeException : Template references a helper that cannot be resolved. Helper 'length'
   at HandlebarsDotNet.Helpers.MissingHelperDescriptor.Invoke(HelperOptions& options, Context& context, Arguments& arguments)
   at HandlebarsDotNet.Helpers.MissingHelperDescriptor.HandlebarsDotNet.Helpers.IHelperDescriptor<HandlebarsDotNet.HelperOptions>.Invoke(HelperOptions& options, Context& context, Arguments& arguments)```

Yep, we have been mentioning this hallucination and handlebar planners imagining the syntax, so they cannot be trusted but as an intermediate step to generate a plan just to review, polish and fix later... but not to use it right away.


text:
Python: Function Calling Stepwise Planner - Max Tokens Ignored after First Call

Describe the bug
I've been experimenting with the Function Calling Stepwise Planner in python (really awesome, thanks for implementing this). I noticed that the first chat completion call respects the max_completion_tokens setting given in the FunctionCallingStepwisePlannerOptions config. However, all the subsequent calls seem to pull from the default from OpenAIPromptExecutionSettings and for me this meant it defaulted back to 256. Function calling seems to consistently require more tokens for its response so this results in responses that terminate with a finish_reason of 'length' and the function call isn't in the right format and things break. Experimenting locally I tweaked the default OpenAIPromptExecutionSettings's max_tokens and I could see that this is what is being followed after the first chat completion.
To Reproduce
Steps to reproduce the behavior:

Use the Function Calling Stepwise Planner with a max_completion_tokens setting other than 256.
Execute any ask (for me I even saw this when the ask was 'hi')
Look at the logs, observe the first and subsequent max_token settings.
Observe the max_completion_tokens setting only honored on the first call.

Expected behavior
FunctionCallingStepwisePlannerOptions's max_completion_tokens followed for all calls.
Screenshots
N/A
Platform

OS: Mac
IDE: Vs Code
Language:Python
Source: main branch of this repo

Additional context
If I have time I can try to investigate further, but right now this is the best I can do!
I was using gpt4 for these tests.

Hi @kevin-m-kent, thank you for reporting this issue. I'll have a look and get a fix in ASAP.

@kevin-m-kent, I've got a PR out for this. Funny enough, we weren't even setting the max_tokens / max_auto_invoke_attemps if configured as part of the function calling stepwise planner options. Thanks for catching this.
I stepped through both of the OpenAI/AzureOpenAI Chat Completions for this planner and confirmed that based on my options, the prompt_execution_settings.max_tokens = 400 were used for all calls. Could you have a look, please? Thanks.

great to hear, thanks! I do see that setting that execution setting does work, but for some reason I only see it on every call after the first (the first for me still defaults to 256). maybe I'm doing something wonky. this is how I'm setting it:
    options = FunctionCallingStepwisePlannerOptions(
        execution_settings=
            AzureChatPromptExecutionSettings(
                max_tokens=400
            )

    )

actually I think my issue with the first call is likely some local version conflict. so I think it's good.

With the change I am proposing, it will give precedence to the FunctionCallingStepwisePlannerOptions max_completion_tokens attribute, if set, and if not, it will use whatever is configured in the OpenAIChatPromptExecutionSettings. Therefore, if you can do one of these:
options = FunctionCallingStepwisePlannerOptions(
    max_iterations=10,
    execution_settings=kernel.get_service(service_id).get_prompt_execution_settings_class()(
        service_id=service_id,max_tokens=1000,
    ),
)

or
    options = FunctionCallingStepwisePlannerOptions(
        max_iterations=10,
        max_tokens=6500,
        execution_settings=kernel.get_service(service_id).get_prompt_execution_settings_class()(
            service_id=service_id,
        ),
    )

Based on the init logic in the planner options, the token usage is computed via some specified/default ratio.

Ah I see ok, I was missing the bit about the init logic. Thanks for explaining

No problem. In any case, I think it's good to establish this order of precedence for setting max_tokens, so it was a great call out by you. Thank you again. As we work on the documentation for v1, we will make sure this gets called out and is clear so all users understand how the configuration needs to be.

great! looking forward to the release.


text:
Python: Upgrade Python notebooks to semantic-kernel v0.3.13

Motivation and Context
Ensure Python notebooks work with semantic-kernel v0.3.13
Description

Contribution Checklist

[ ] The code builds clean without any errors or warnings
[ ] The PR follows the SK Contribution Guidelines and the pre-submission formatting script raises no violations
[ ] All unit tests pass, and I have added new tests where possible
[ ] I didn't break anyone :smile:

Any reason not to use v0.3.14 since that's the latest?

authors: alexchaomander markwallace-microsoft


text:
ActionPlanner: adopt SK's config methods for registering models

SK uses the pattern of kernel.Config.AddAzureTextCompletionService() to configure the models that should be used. The ActionPlanner should adopt a similar pattern to better align with SK style prompts. So in JS you could add a model using planner.addAzureTextCompletionService()

P0

Closing since we have forked away from SK

authors: Stevenic corinagum govil1811

repo: microsoft/teams-ai


text:
Always-on-top quake mode window

Description of the new feature/enhancement
The way I use the _quake window (for a one-off command or series of commands), it usually has my attention for as long as it's up, but on the occasions where I turn away to do other things, it can easily get lost in the mess and/or confused with one of my regular terminal windows. Having it always-on-top such that it is either dropped-down on-top or not makes this less likely to happen in my workflow.
Proposed technical implementation details (optional)
While just making the _quake window always-on-top by default would fit the letter of the request, I'd much rather have always-on-topness added as a parameter that can be used with quakeMode and globalSummon to influence the parameters of windows they create. It would also be useful to have it as a command-line option for use with -w -- I currently use a _C:\Users\avatar\AppData\Local\Microsoft\WindowsApps\wt.exe -w _quake to automatically open the _quake window when I log on, and while I can toggle on always-on-topness after opening it, I would prefer to be able to have that in the shortcut.

If you have the "AlwaysOnTop" enabled in your settings.json, does launching via quake mode ignore this?  If it does, then I'd call it dupe https://github.com/microsoft/terminal/issues/9992

Yea, this is gonna fall under /dup #9992

authors: WSLUser cerebrate zadjii-msft

repo: microsoft/terminal


text:
Paste without warning about newline

Hi!
While it's great that Terminal warns if I paste something that contains newlines, in some cases this is done intentionally, and in these cases the warning is a bit annoying.
Is there a way, e.g. a custom action, to paste without this warning?

Good news, there is, please check out the docs.

Sorry for bothering again. This feature is helpful, but for my usecase it would be better if I could have the warning enabled, but also have the possibility to disable it on demand, e.g. with a certain keyboard shortcut bound to some action. Do you think this is feasible?

authors: hensz zadjii-msft


text:
Terminal can't be indexed by powertoys

Windows Terminal version
1.12.10334.0
Windows build number
10.0.19043.0
Other Software
No response
Steps to reproduce
press alt + space.
enter terminal
Expected Behavior
I can see Terminal in the list as early release.
Actual Behavior
I can't see Terminal in the list.

This is a /duplicate of https://github.com/microsoft/PowerToys/issues/16118

(Thanks got the report!)

authors: DH-Illidan DHowett


text:
DSC Resource for Windows Terminal

Description of the new feature/enhancement
I would like a DSC Resource so I can specify the way I want Windows Terminal configured. WinGet Configuration or the Dev Home experience could leverage a configuration file to set Windows Terminal up the way I want it.
Proposed technical implementation details (optional)
Some notes from discussion: Paths in the json file will be Hard. If someone's got a relative path in their settings.json, well, that just won't work on another machine.
The ideal case here is probably a dotfiles-like experience where we can DSC that folder of fonts, images, and settings.json.
Crawl

We can install the Terminal.

Walk

We can make some customizations to the settings.json. Maybe use a well-known URL to apply your settings.

Run

We can also install fragment extensions the user already has.
We can install distros, PWSH's etc.

Sprint

We can export the current settings.json to a dotfiles-like folder, with images, fonts, etc, and re-write the settings.json to use that path

As discussed at Build last week:

actions, schemes, themes: These are all pretty much machine-independent. (there may be minimal actions that reference profiles, but the 99% case should work fine).
fonts out of profiles could be enumerated and included in the resource
fragment extensions could also be enumerated and included in the list of dependencies

Similarly with PWSHs, WSLs, VS, and other dynamic profiles that we can enumerate

profiles are very much machine-dependent.

As a crawl, we could just include them with no modifications
walk: include them, but with some property that indicates they've been migrated / restored somehow, and the user should manually fix them?
run: We could be smarter about "migrated" profiles, and default to some sensible CWD if we can't find the startingDirectory for that profile
sprint: When the user opens a migrated profile the first time, pop a dialog to confirm the choice of what the commandline & starting directory should be.

newTabMenu is vaguely machine dependent, in the sense that it relies on the existence of profiles. But I think we're smart enough to leave entries that don't point at an existing profile out of the new tab menu. We may be able to migrate it wholesale with minimal concern for if the profiles in it migrated successfully.

authors: denelon zadjii-msft