Merge branch 'dev' for release

Author: abyrd

.github/ISSUE_TEMPLATE/bug_report.md

@@ -0,0 +1,38 @@
+---
+name: Bug report
+about: Create a report to help us improve
+title: ''
+labels: ''
+assignees: ''
+
+---
+
+**Describe the bug**
+A clear and concise description of what the bug is.
+
+**To Reproduce**
+Steps to reproduce the behavior:
+1. Go to '...'
+2. Click on '....'
+3. Scroll down to '....'
+4. See error
+
+**Expected behavior**
+A clear and concise description of what you expected to happen.
+
+**Screenshots**
+If applicable, add screenshots to help explain your problem.
+
+**Desktop (please complete the following information):**
+ - OS: [e.g. iOS]
+ - Browser [e.g. chrome, safari]
+ - Version [e.g. 22]
+
+**Smartphone (please complete the following information):**
+ - Device: [e.g. iPhone6]
+ - OS: [e.g. iOS8.1]
+ - Browser [e.g. stock browser, safari]
+ - Version [e.g. 22]
+
+**Additional context**
+Add any other context about the problem here.

.github/ISSUE_TEMPLATE/feature_request.md

@@ -0,0 +1,20 @@
+---
+name: Feature request
+about: Suggest an idea for this project
+title: ''
+labels: ''
+assignees: ''
+
+---
+
+**Is your feature request related to a problem? Please describe.**
+A clear and concise description of what the problem is. Ex. I'm always frustrated when [...]
+
+**Describe the solution you'd like**
+A clear and concise description of what you want to happen.
+
+**Describe alternatives you've considered**
+A clear and concise description of any alternative solutions or features you've considered.
+
+**Additional context**
+Add any other context or screenshots about the feature request here.

.github/workflows/codeql-analysis.yml

@@ -0,0 +1,73 @@
+# This file was created by CodeQL for this repository. The only change was 
+# removing 'python' and 'javascript' from the language array.
+#
+# For most projects, this workflow file will not need changing; you simply need
+# to commit it to your repository.
+#
+# You may wish to alter this file to override the set of languages analyzed,
+# or to provide custom queries or build logic.
+#
+# ******** NOTE ********
+# We have attempted to detect the languages in your repository. Please check
+# the `language` matrix defined below to confirm you have the correct set of
+# supported CodeQL languages.
+#
+name: "CodeQL"
+
+on:
+  push:
+    branches: [ dev ]
+  pull_request:
+    # The branches below must be a subset of the branches above
+    branches: [ dev ]
+  schedule:
+    - cron: '24 12 * * 1'
+
+jobs:
+  analyze:
+    name: Analyze
+    runs-on: ubuntu-latest
+    permissions:
+      actions: read
+      contents: read
+      security-events: write
+
+    strategy:
+      fail-fast: false
+      matrix:
+        language: [ 'java' ]
+        # CodeQL supports [ 'cpp', 'csharp', 'go', 'java', 'javascript', 'python', 'ruby' ]
+        # Learn more about CodeQL language support at https://aka.ms/codeql-docs/language-support
+
+    steps:
+    - name: Checkout repository
+      uses: actions/checkout@v3
+
+    # Initializes the CodeQL tools for scanning.
+    - name: Initialize CodeQL
+      uses: github/codeql-action/init@v2
+      with:
+        languages: ${{ matrix.language }}
+        # If you wish to specify custom queries, you can do so here or in a config file.
+        # By default, queries listed here will override any specified in a config file.
+        # Prefix the list here with "+" to use these queries and those in the config file.
+        # queries: ./path/to/local/query, your-org/your-repo/queries@main
+
+    # Autobuild attempts to build any compiled languages  (C/C++, C#, or Java).
+    # If this step fails, then you should remove it and run the build manually (see below)
+    - name: Autobuild
+      uses: github/codeql-action/autobuild@v2
+
+    # ℹ️ Command-line programs to run using the OS shell.
+    # 📚 See https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#jobsjob_idstepsrun
+
+    # ✏️ If the Autobuild fails above, remove it and uncomment the following three lines
+    #    and modify them (or add more) to build your code if your project
+    #    uses a compiled language
+
+    #- run: |
+    #   make bootstrap
+    #   make release
+
+    - name: Perform CodeQL Analysis
+      uses: github/codeql-action/analyze@v2

build.gradle

@@ -123,10 +123,8 @@ repositories {
     // Put Open Source Geospatial before Maven Central to get JAI core, see https://stackoverflow.com/a/26993223
     maven { url 'https://repo.osgeo.org/repository/release/' }
     mavenCentral()
-    // TODO review whether we really need these repositories
+    // TODO review whether we really need the repositories below
     maven { url 'https://maven.conveyal.com' }
-    // Used for importing java projects from github (why do we need this?)
-    maven { url 'https://jitpack.io' }
     // For the polyline encoder
     maven { url 'https://nexus.axiomalaska.com/nexus/content/repositories/public-releases' }
 }

src/main/java/com/conveyal/analysis/components/broker/Broker.java

@@ -1,6 +1,5 @@
 package com.conveyal.analysis.components.broker;
 
-import com.conveyal.analysis.AnalysisServerException;
 import com.conveyal.analysis.components.Component;
 import com.conveyal.analysis.components.WorkerLauncher;
 import com.conveyal.analysis.components.eventbus.ErrorEvent;
@@ -108,7 +107,8 @@ public interface Config {
      * Used when auto-starting spot instances. Set to a smaller value to increase the number of
      * workers requested automatically
      */
-    public final int TARGET_TASKS_PER_WORKER = 800;
+    public final int TARGET_TASKS_PER_WORKER_TRANSIT = 800;
+    public final int TARGET_TASKS_PER_WORKER_NONTRANSIT = 4_000;
 
     /**
      * We want to request spot instances to "boost" regional analyses after a few regional task
@@ -243,28 +243,54 @@ public void createOnDemandWorkerInCategory(WorkerCategory category, WorkerTags w
     /**
      * Create on-demand/spot workers for a given job, after certain checks
      * @param nOnDemand EC2 on-demand instances to request
-     * @param nSpot EC2 spot instances to request
+     * @param nSpot Target number of EC2 spot instances to request. The actual number requested may be lower if the
+     *              total number of workers running is approaching the maximum specified in the Broker config.
      */
     public void createWorkersInCategory (WorkerCategory category, WorkerTags workerTags, int nOnDemand, int nSpot) {
 
+        // Log error messages rather than throwing exceptions, as this code often runs in worker poll handlers.
+        // Throwing an exception there would not report any useful information to anyone.
+
         if (config.offline()) {
-            LOG.info("Work offline enabled, not creating workers for {}", category);
+            LOG.info("Work offline enabled, not creating workers for {}.", category);
+            return;
+        }
+
+        if (nOnDemand < 0 || nSpot < 0) {
+            LOG.error("Negative number of workers requested, not starting any.");
+            return;
+        }
+
+        final int nRequested = nOnDemand + nSpot;
+        if (nRequested <= 0) {
+            LOG.error("No workers requested, not starting any.");
             return;
         }
 
-        if (nOnDemand < 0 || nSpot < 0){
-            LOG.info("Negative number of workers requested, not starting any");
+        // Zeno's worker pool management: never start more than half the remaining capacity.
+        final int remainingCapacity = config.maxWorkers() - workerCatalog.totalWorkerCount();
+        final int maxToStart = remainingCapacity / 2;
+        if (maxToStart <= 0) {
+            LOG.error("Due to capacity limiting, not starting any workers.");
             return;
         }
 
-        if (workerCatalog.totalWorkerCount() + nOnDemand + nSpot >= config.maxWorkers()) {
-            String message = String.format(
-                    "Maximum of %d workers already started, not starting more;" +
-                    "jobs will not complete on %s",
-                    config.maxWorkers(),
-                    category
+        if (nRequested > maxToStart) {
+            LOG.warn("Request for {} workers is more than half the remaining worker pool capacity.", nRequested);
+            nSpot = maxToStart;
+            nOnDemand = 0;
+            LOG.warn("Lowered to {} on-demand and {} spot workers.", nOnDemand, nSpot);
+        }
+
+        // Just an assertion for consistent state - this should never happen.
+        // Re-sum nOnDemand + nSpot here instead of using nTotal, as they may have been revised.
+        if (workerCatalog.totalWorkerCount() + nOnDemand + nSpot > config.maxWorkers()) {
+            LOG.error(
+                "Starting workers would exceed the maximum capacity of {}. Jobs may stall on {}.",
+                config.maxWorkers(),
+                category
             );
-            throw AnalysisServerException.forbidden(message);
+            return;
         }
 
         // If workers have already been started up, don't repeat the operation.
@@ -483,9 +509,27 @@ private void requestExtraWorkersIfAppropriate(Job job) {
         WorkerCategory workerCategory = job.workerCategory;
         int categoryWorkersAlreadyRunning = workerCatalog.countWorkersInCategory(workerCategory);
         if (categoryWorkersAlreadyRunning < MAX_WORKERS_PER_CATEGORY) {
-            // Start a number of workers that scales with the number of total tasks, up to a fixed number.
-            // TODO more refined determination of number of workers to start (e.g. using tasks per minute)
-            int targetWorkerTotal = Math.min(MAX_WORKERS_PER_CATEGORY, job.nTasksTotal / TARGET_TASKS_PER_WORKER);
+            // TODO more refined determination of number of workers to start (e.g. using observed tasks per minute
+            //  for recently completed tasks -- but what about when initial origins are in a desert/ocean?)
+            int targetWorkerTotal;
+            if (job.templateTask.hasTransit()) {
+                // Total computation for a task with transit depends on the number of stops and whether the
+                // network has frequency-based routes. The total computation for the job depends on these
+                // factors as well as the number of tasks (origins). Zoom levels add a complication: the number of
+                // origins becomes an even poorer proxy for the number of stops. We use a scale factor to compensate
+                // -- all else equal, high zoom levels imply fewer stops per origin (task) and a lower ideal target
+                // for number of workers. TODO reduce scale factor further when there are no frequency routes. But is
+                //  this worth adding a field to Job or RegionalTask?
+                float transitScaleFactor = (9f / job.templateTask.zoom);
+                targetWorkerTotal = (int) ((job.nTasksTotal / TARGET_TASKS_PER_WORKER_TRANSIT) * transitScaleFactor);
+            } else {
+                // Tasks without transit are simpler. They complete relatively quickly, and the total computation for
+                // the job increases roughly with linearly with the number of origins.
+                targetWorkerTotal = job.nTasksTotal / TARGET_TASKS_PER_WORKER_NONTRANSIT;
+            }
+
+            // Do not exceed the limit on workers per category TODO add similar limit per accessGroup or user
+            targetWorkerTotal = Math.min(targetWorkerTotal, MAX_WORKERS_PER_CATEGORY);
             // Guardrail until freeform pointsets are tested more thoroughly
             if (job.templateTask.originPointSet != null) targetWorkerTotal = Math.min(targetWorkerTotal, 5);
             int nSpot =  targetWorkerTotal - categoryWorkersAlreadyRunning;

src/main/java/com/conveyal/analysis/controllers/WorkerProxyController.java

@@ -23,8 +23,6 @@
 import java.net.http.HttpRequest;
 import java.net.http.HttpResponse;
 
-import static com.conveyal.analysis.util.HttpStatus.OK_200;
-
 /**
  * This proxies requests coming from the UI over to any currently active worker for the specified network bundle.
  * This could be used for point-to-point routing or the existing R5 endpoints producing debug tiles of the graph.
@@ -104,7 +102,7 @@ private Object proxyRequest (Request request, Response response) throws IOExcept
             resp.headers().map().forEach((key, value) -> {
                 if (!value.isEmpty()) response.header(key, value.get(0));
             });
-            response.status(OK_200);
+            response.status(resp.statusCode());
             return resp.body();
         } catch (Exception exception) {
             response.status(HttpStatus.BAD_REQUEST_400);

src/main/java/com/conveyal/r5/analyst/FreeFormPointSet.java

@@ -123,7 +123,7 @@ public static FreeFormPointSet fromCsv (
             return ret;
         } catch (NumberFormatException nfe) {
             throw new ParameterException(
-                String.format("Improperly formatted floating point value on line %d of CSV input", rec)
+                String.format("Improperly formatted floating point value near line %d of CSV input", rec + 1)
             );
         }
     }

src/main/java/com/conveyal/r5/analyst/cluster/AnalysisWorker.java

@@ -38,7 +38,6 @@
 import java.time.format.DateTimeFormatter;
 import java.util.ArrayList;
 import java.util.Arrays;
-import java.util.Collections;
 import java.util.List;
 import java.util.Random;
 import java.util.UUID;
@@ -348,7 +347,7 @@ private byte[] singlePointResultToBinary (
                     oneOriginResult.accessibility,
                     transportNetwork.scenarioApplicationWarnings,
                     transportNetwork.scenarioApplicationInfo,
-                    oneOriginResult.paths
+                    oneOriginResult.paths != null ? new PathResultSummary(oneOriginResult.paths, transportNetwork.transitLayer) : null
             );
         }
         // Single-point tasks don't have a job ID. For now, we'll categorize them by scenario ID.
@@ -488,7 +487,7 @@ public static class GridJsonBlock {
 
         public List<TaskError> scenarioApplicationInfo;
 
-        public List<PathResult.PathIterations> pathSummaries;
+        public PathResultSummary pathSummaries;
 
         @Override
         public String toString () {
@@ -515,7 +514,7 @@ public static void addJsonToGrid (
             AccessibilityResult accessibilityResult,
             List<TaskError> scenarioApplicationWarnings,
             List<TaskError> scenarioApplicationInfo,
-            PathResult pathResult
+            PathResultSummary pathResult
     ) throws IOException {
         var jsonBlock = new GridJsonBlock();
         jsonBlock.scenarioApplicationInfo = scenarioApplicationInfo;
@@ -526,7 +525,7 @@ public static void addJsonToGrid (
             // study area). But we'd need to control the number of decimal places serialized into the JSON.
             jsonBlock.accessibility = accessibilityResult.getIntValues();
         }
-        jsonBlock.pathSummaries = pathResult == null ? Collections.EMPTY_LIST : pathResult.getPathIterationsForDestination();
+        jsonBlock.pathSummaries = pathResult;
         LOG.debug("Travel time surface written, appending {}.", jsonBlock);
         // We could do this when setting up the Spark handler, supplying writeValue as the response transformer
         // But then you also have to handle the case where you are returning raw bytes.