Summary:
Sometimes the model repeats the classes given in the prompt which would mess up extracting the actual formula. This diff solves this by:
1. Changes the generated Python schema so that (a) the thing that needs completing is a plain top level function instead of a property/method inside the class and (2) the classes are fully valid syntax, which makes it easier to
2. Remove classes from the parsed Python code when converting the completion to a formula.
3. Tweak the prompt wording to discourage including classes in general, especially because sometimes the model tries to solve the problem by defining extra methods/attributes/classes.
While I was at it, I changed type hints to use builtins (e.g. `list` instead of `List`) to prevent `from typing import List` which was happening sometimes and would look weird in a formula. Similarly I removed `@dataclass` since that also implies an import, and this also fits with the tweaked wording that the classes are fake.
Test Plan:
Added a new test case to the formula dataset which triggers the unwanted behaviour. The factors that seem to trigger the problem are (1) a small schema so the classes are easier to repeat and (2) the need to import modules, which the model wants to place before all other code. The case failed before this diff and succeeded after. The tweaked wording reduces the chances of repeating the classes but didn't eliminate it, so forcibly removing the classes in Python was needed.
There were also a couple of other existing cases where repeating the classes was observed before but not after.
Overall the score increased from 49 to 51 out of 69 (including the new case). At one point the score was 53, but changes in whitespace were enough to make it drop again.
Reviewers: georgegevoian
Reviewed By: georgegevoian
Differential Revision: https://phab.getgrist.com/D4000
* REQUEST now supports POST
* Add extra flag for enabling REQUEST, also update README and comments
Co-authored-by: John Cant <a.jonncant@gmail.com>
Co-authored-by: Alex Hall <alex.mojaki@gmail.com>
Summary: Mostly just changes to tests to make them more flexible.
Test Plan: Python tests pass locally with 3.10 and 3.11. Making tests run in CI on these versions will happen in grist-core.
Reviewers: paulfitz
Reviewed By: paulfitz
Subscribers: paulfitz
Differential Revision: https://phab.getgrist.com/D3978
Summary:
Upgrades asttokens mainly because I suspected it would have helped with an error in a user's document (see https://grist.slack.com/archives/C0234CPPXPA/p1686145370484509) but we were unable to confirm that. Also adds a related test that I (wrongly) expected to have a similar problem before upgrading asttokens.
Upgrades wrapt for Python 3.11 support. Closes https://github.com/gristlabs/grist-core/issues/534.
Not upgrading the dependencies for Python 2 because pip is giving me errors when I try to install to the `thirdparty` folder. Both these upgrades are motivated by issues specific to Python 3 so this doesn't seem worth pursuing immediately.
Test Plan: Existing tests.
Reviewers: paulfitz
Reviewed By: paulfitz
Subscribers: paulfitz
Differential Revision: https://phab.getgrist.com/D3973
Summary:
Replaces https://phab.getgrist.com/D3940, particularly to avoid doing potentially unwanted things automatically.
Adds optional fields `evaluateCurrentFormula?: boolean; rowId?: number` to `FormulaAssistanceContext` (part of `AssistanceRequest`). When `evaluateCurrentFormula` is `true`, calls a new function `evaluate_formula` in the sandbox which computes the existing formula in the column (regardless of anything the AI may have suggested) and uses that to generate an additional system message which is added before the user's message. In theory this could be used in an interface where users ask why a formula doesn't work, including possibly a formula suggested by the AI. For now, it's only used in `runCompletion_impl.ts` for experimenting.
Also cleaned up a bit, removing `_chatMode` which is always `true` now, and uses of `regenerate` which is always `false`.
Test Plan: Updated `runCompletion_impl` to optionally use the new feature, in which case it now scores 51/68 instead of 49/68.
Reviewers: paulfitz
Reviewed By: paulfitz
Differential Revision: https://phab.getgrist.com/D3970
Summary:
The image Jenkins used up to now was from 2021. We updated buildtools/jenkins-ec2/Dockerfile since then but couldn't use new image; we can now that an issue with mounts in gvisor has been resolved.
The new image uses newer Chrome (111 vs 95); 111 is the latest version that works with the webdriver version we are using (after that one, copy-pasting tests fail).
In addition, this adds a more precise way to maintain python dependencies: by specifying top-level dependencies in the new core/sandbox/requirements3.in. An updated build step compiles that into requirements3.txt, and syncs venv to that using pip-sync.
This addresses the issue that previously, removing a module from dependencies would not have caused the build to remove it from sandbox_venv3.
Test Plan: Existing tests pass on the new image.
Reviewers: georgegevoian
Reviewed By: georgegevoian
Differential Revision: https://phab.getgrist.com/D3952
Summary:
- Replace logger module by the standard module 'logging'.
- When a log message from the sandbox includes newlines (e.g. for tracebacks),
keep those lines together in the Node log message.
Previously each line was a different message, making it difficult to view
tracebacks, particularly in prod where each line becomes a separate message
object.
- Fix assorted lint errors.
Test Plan: Added a test for the log-line splitting and escaping logic.
Reviewers: georgegevoian
Reviewed By: georgegevoian
Differential Revision: https://phab.getgrist.com/D3956
Summary:
This tweaks the prompting so that the user's message is given on its own instead of as a docstring within Python. This is so that the prompt makes sense when:
- the user asks a question such as "Can you write me a formula which does ...?" rather than describing their formula as a docstring would, or
- the user sends a message that doesn't ask for a formula at all (https://grist.slack.com/archives/C0234CPPXPA/p1687699944315069?thread_ts=1687698078.832209&cid=C0234CPPXPA)
Also added wording for the model to refuse when the user asks for something that the model cannot do.
Because the code (and maybe in some cases the model) for non-ChatGPT models relies on the prompt consisting entirely of Python code produced by the data engine (which no longer contains the user's message) those code paths have been disabled for now. Updating them now seems like undesirable drag, I think it'd be better to revisit this when iteration/experimentation has slowed down and stabilised.
Test Plan:
Added entries to the formula dataset where the response shouldn't contain a formula, indicated by the value `1` for the new column `no_formula`.
This is somewhat successful, as the model does refuse to help in some of the new test cases, but not all. Performance on existing entries also seems a bit worse, but it's hard to distinguish this from random noise. Hopefully this can be remedied in the future with more work, e.g. automatic followup messages containing example inputs and outputs.
Reviewers: paulfitz
Reviewed By: paulfitz
Subscribers: dsagal
Differential Revision: https://phab.getgrist.com/D3936
Summary: Converts `rec.` to `$` in AI generated formulas, and removes redundant `return` at the end.
Test Plan: Expanded unit test.
Reviewers: dsagal
Reviewed By: dsagal
Differential Revision: https://phab.getgrist.com/D3933
Summary:
The previous code for extracting a Python formula from the LLM completion involved some shaky string manipulation which this improves on.
Overall the 'test results' from `runCompletion` went from 37/47 to 45/47 for `gpt-3.5-turbo-0613`.
The biggest problem that motivated these changes was that it assumed that code was always inside a markdown code block
(i.e. triple backticks) and so if there was no block there was no code. But the completion often consists of *only* code
with no accompanying explanation or markdown. By parsing the completion in Python instead of JS,
we can easily check if the entire completion is valid Python syntax and accept it if it is.
I also noticed one failure resulting from the completion containing the full function (instead of just the body)
and necessary imports before that function instead of inside. The new parsing moves import inside.
Test Plan: Added a Python unit test
Reviewers: paulfitz
Reviewed By: paulfitz
Subscribers: paulfitz
Differential Revision: https://phab.getgrist.com/D3922
Summary:
In the future, it may be helpful to capture some output to display in the
formula editor (e.g. the result of calling print()), but for now it's best
to redirect stdout/stderr to avoid excessive logging.
Test Plan: Tested manually.
Reviewers: alexmojaki
Reviewed By: alexmojaki
Subscribers: alexmojaki
Differential Revision: https://phab.getgrist.com/D3919
Summary:
For grist-static, we want to the data engine to be able to call external/exported JS functions directly,
rather than via the node 'server' living in another thread which requires synchronous communication hackery.
As a step in that direction, this diff changes the exported functions that we care about (guessColInfo and convertFromColumn)
to just using the top-level functions instead of relying on fields in ActiveDoc, namely docData.
For guessColInfo, this is done by directly passing the small amount of metadata that was previously retrieved from the DocData.
For convertFromColumn, disentangling DocData is a lot more complicated, so instead we construct a fresh DocData object using
the required metadata tables which are now passed in by the data engine.
Test Plan: Existing tests
Reviewers: paulfitz
Reviewed By: paulfitz
Differential Revision: https://phab.getgrist.com/D3913
Summary:
Due to a mishap, two distinct migrations with the same migration
number were introduced into Grist. This diff reconciles them as
best we can, by adding another migration to make sure both desired
changes have run (and running them if not).
Test Plan:
updated a test; checked manually that documents
with different 38 migrations are handled as expected.
Reviewers: georgegevoian, jarek
Reviewed By: georgegevoian, jarek
Differential Revision: https://phab.getgrist.com/D3895
Summary:
This adds a UI panel for managing webhooks. Work started by Cyprien Pindat. You can find the UI on a document's settings page. Main changes relative to Cyprien's demo:
* Changed behavior of virtual table to be more consistent with the rest of Grist, by factoring out part of the implementation of on-demand tables.
* Cell values that would create an error can now be denied and reverted (as for the rest of Grist).
* Changes made by other users are integrated in a sane way.
* Basic undo/redo support is added using the regular undo/redo stack.
* The table list in the drop-down is now updated if schema changes.
* Added a notification from back-end when webhook status is updated so constant polling isn't needed to support multi-user operation.
* Factored out webhook specific logic from general virtual table support.
* Made a bunch of fixes to various broken behavior.
* Added tests.
The code remains somewhat unpolished, and behavior in the presence of errors is imperfect in general but may be adequate for this case.
I assume that we'll soon be lifting the restriction on the set of domains that are supported for webhooks - otherwise we'd want to provide some friendly way to discover that list of supported domains rather than just throwing an error.
I don't actually know a lot about how the front-end works - it looks like tables/columns/fields/sections can be safely added if they have string ids that won't collide with bone fide numeric ids from the back end. Sneaky.
Contains a migration, so needs an extra reviewer for that.
Test Plan: added tests
Reviewers: jarek, dsagal
Reviewed By: jarek, dsagal
Differential Revision: https://phab.getgrist.com/D3856
* add support for conversational state to assistance endpoint
This refactors the assistance code somewhat, to allow carrying
along some conversational state. It extends the OpenAI-flavored
assistant to make use of that state to have a conversation.
The front-end is tweaked a little bit to allow for replies that
don't have any code in them (though I didn't get into formatting
such replies nicely).
Currently tested primarily through the runCompletion script,
which has been extended a bit to allow testing simulated
conversations (where an error is pasted in follow-up, or
an expected-vs-actual comparison).
Co-authored-by: George Gevoian <85144792+georgegevoian@users.noreply.github.com>
Summary:
- When importing into a Ref column, use lookupOne() formula for correct previews.
- When selecting columns to import into a Ref column, now a Numeric column like
'Order' will produce two options: "Order" and "Order (as row ID)".
- Fixes exports to correct the formatting of visible columns. This addresses multiple bugs:
1. Formatting wasn't used, e.g. a Ref showing a custom-formatted date was still presented as YYYY-MM-DD in CSVs.
2. Ref showing a Numeric column was formatted as if a row ID (e.g. `Table1[1.5]`), which is very wrong.
- If importing into a table that doesn't have a primary view, don't switch page after import.
Refactorings:
- Generalize GenImporterView to be usable in more cases; removed near-duplicated logic from node side
- Some other refactoring in importing code.
- Fix field/column option selection in ValueParser
- Add NUM() helper to turn integer-valued floats into ints, useful for "as row ID" lookups.
Test Plan: Added test cases for imports into reference columns, updated Exports test fixtures.
Reviewers: georgegevoian
Reviewed By: georgegevoian
Differential Revision: https://phab.getgrist.com/D3875
Summary:
This addresses two issues, differently:
- For a formula with leading whitespace, like " 1+1", it is stored as is, but
is fixed to work (it should be valid Python, and whitespace is only stripped out
at parsing time to avoid intentation errors caused by the way it gets parsed)
- For a formula with a leading equals-sign ("="), it is stripped out on the
client side before the formula is stored. Grist documentation uses leading
"=" to indicate formulas (because UI shows an "=" icon), and Excel formulas
actually contain the leading "=", so it is a common mistake to include it.
Test Plan: Added new test cases
Reviewers: jarek
Reviewed By: jarek
Differential Revision: https://phab.getgrist.com/D3873
Summary: Also adds an explanatory comment for a recently-added column.
Test Plan: N/A
Reviewers: jarek
Reviewed By: jarek
Differential Revision: https://phab.getgrist.com/D3854
Summary:
When a formula used a local variable referring to a user table (which is
a global name), e.g. `myvar = MyTable; myvar.lookupOne(...)`, renaming
logic mistakenly used the inferred name (`MyTable`) in places where the
actual variable name (`myvar`) should have been used.
Additionally, we should not rename local variables, even if they match a
global name.
This fixes both issues.
Test Plan: Added a test case that checks that local variables aren't renamed.
Reviewers: georgegevoian
Reviewed By: georgegevoian
Differential Revision: https://phab.getgrist.com/D3846
Test Plan: Added a test case to trigger this situation.
Reviewers: georgegevoian
Reviewed By: georgegevoian
Differential Revision: https://phab.getgrist.com/D3832
Summary:
The problem is that the implementation for a summary update was relying on type consistency to get columns (ie: matches agains colId and type).
Type consistency is an attempt at maintaining consistent type across same-named column for summaries of same table.
But the problem is that the consistency of types is NOT a strict guarantee or an invariant, more of a best-effort attempt (there are too many possible sequences of operations possible with renaming/adding/removing in summary tables and the underlying table).
With current implementation and with a document violating the type consistency, a summary table could end up with fields referencing columns to the former summary table (more detail below(1)). Which is a bad state (yields js errors on the client).
This diff fixes this issue by relaxing the type comparison when search for same-named column.
(1) __Below is a description of how a violation of type consistency could end-up in bad state document (example taken from the reported bug):__
> In this document, let's assume two summary tables `Table1 [by A]` and `Table1 [Totals]`. Let's also assume Table1 and `Table1 [Totals]` both have an `Amount(Numeric)` column, and that `Table1 [by A]` has one `Amount(Any)` column (violating the type consistency principle). Now when users wanted to change the `Table1 [Totals]` section to group by 'A', grist found that there is already a summary table with same grouping. But it couldn't find a matching column for `Amount(Numeric)` so it created a new one. Except that because there was still an `Amount(Any)` the new column was named `Amount2` which caused following code to ignore it and in particular forgetting to update it's corresponding section's field which was then pointing toward the column of a different table (which is bad).
Test Plan: Added python test.
Reviewers: georgegevoian
Reviewed By: georgegevoian
Differential Revision: https://phab.getgrist.com/D3809
Summary:
- For python2, skip some tests of renaming which produce different results
because of an un-upgradable astroid version.
- Fix test affected by pyCall() having changed to async; avoid hanging timeout
callback in case of error.
Test Plan: All test cases should now pass (with 4 getting skipped)
Reviewers: paulfitz
Reviewed By: paulfitz
Differential Revision: https://phab.getgrist.com/D3819
This adds a new `GRIST_SANDBOX_FLAVOR=pyodide` option where the
version of Python used for the data engine is wasm, and so can
be run by node like the rest of the back end. It still runs as
a separate process.
There are a few small version changes made to packages to avoid
various awkwardnesses present in the current versions. All existing
tests pass.
This is very experimental. To use, you'll need something with
a bash shell and make. First do:
```
cd sandbox/pyodide
make setup # README.md and Makefile have details
cd ..
```
Then running Grist as:
```
GRIST_SANDBOX_FLAVOR=pyodide yarn start
```
should work. Adding a formula with content:
```
import sys; return sys.version
```
should return a different Python version than other sandboxes.
The motivation for this work is to have a form of sandboxing
that will work on Windows for Grist Electron (for Linux we have
gvisor/runsc, for Mac we have sandbox-exec, but I haven't found
anything comparable for Windows).
It also brings a back-end-free version of Grist a bit closer, for
use-cases where that would make sense - such as serving a report
(in the form of a Grist document) on a static site.
Summary:
Whole numbers, when imported from Excel into a Text column show up
without decimals (e.g. "300"), but when imported from Google Sheets show
up with decimals (e.g. "300.0"). The decimals are hard for end-users to
remove. Fix by treating whole numbers consistently as ints.
Test Plan: Added a fixture reproducing the issue, and a test case.
Reviewers: georgegevoian
Reviewed By: georgegevoian
Differential Revision: https://phab.getgrist.com/D3800
Summary:
- A bunch of optimizations guided by python profiling (esp. py-spy)
- Big one is optimizing Record/RecordSet attribute access
- Adds tracemalloc printout when running test_replay with PYTHONTRACEMALLOC=1 (on PY3)
(but memory size is barely affected by these changes)
- Testing with RECORD_SANDBOX_BUFFERS_DIR, loading and calculating a particular
very large doc (CRM), time taken improved from 73.9s to 54.8s (26% faster)
Test Plan: No behavior changes intended; relying on existing tests to verify that.
Reviewers: georgegevoian
Reviewed By: georgegevoian
Differential Revision: https://phab.getgrist.com/D3781
Summary: This is a backend part for the formula AI.
Test Plan: New tests
Reviewers: paulfitz
Reviewed By: paulfitz
Subscribers: cyprien
Differential Revision: https://phab.getgrist.com/D3786
There was no script for updating typescript schema information after
a python-based document migration. Moving one in here, along with its
test. Tweaked the code slightly to work with grist-core's directory
structure. Also fixed a formatting error in mocha calls that was resulting
in some root tests not running.
Summary:
Fix for a bug that prevented two users to change column types at
the same time.
Test Plan: Added and updated
Reviewers: georgegevoian
Reviewed By: georgegevoian
Differential Revision: https://phab.getgrist.com/D3745
Summary:
for users who don't automatically have deep rights
to the document, provide them with attachment metadata only
for rows they have access to. This is a little tricky to
do efficiently. We provide attachment metadata when an
individual table is fetched, rather than on initial document
load, so we don't block that load on a full document scan.
We provide attachment metadata to a client when we see that
we are shipping rows mentioning particular attachments,
without making any effort to keep track of the metadata they
already have.
Test Plan: updated tests
Reviewers: dsagal, jarek
Reviewed By: dsagal, jarek
Differential Revision: https://phab.getgrist.com/D3722
Summary:
This is just a convenience for myself. I happen to have a version of
gvisor on my Linux dev machine that differs from what we use in our
containers. There's a small difference in user setup that only manifests
itself when importing files. Grist uses a directory readable only by
the creating user, created outside the container, and then accessed
within the container. For that to work, the user identities have to
line up exactly. This adds a flag I can set in my environment to make
things work. An alternative solution that doesn't require a flag
would be to make the temporary directories readable by other users,
but that seemed a bigger change than justified.
Ideally we'd make a very robust and easy to run sandbox for Linux
users, and I have ideas there for the future.
Test Plan: manual
Reviewers: dsagal
Reviewed By: dsagal
Differential Revision: https://phab.getgrist.com/D3742
Summary:
Adds a new UI for writing access rule memos.
Migrates old memos (written as Python comments) to the new UI.
Test Plan: Browser and migration tests.
Reviewers: jarek, dsagal
Reviewed By: jarek
Subscribers: dsagal, paulfitz
Differential Revision: https://phab.getgrist.com/D3726
Summary:
Rows in the _grist_Attachments table have a special lifecycle,
being created by a special method, and deleted via a special
process. All other modifications are now rejected, for simplicity.
Test Plan: added test
Reviewers: dsagal, jarek
Reviewed By: dsagal, jarek
Differential Revision: https://phab.getgrist.com/D3712
Summary:
The sort and filter UI now has a more unified UI, with similar
capabilities that are accessible from different parts of Grist.
It's now also possible to pin individual filters to the filter bar,
which replaces the old toggle for showing all filters in the
filter bar.
Test Plan: Various tests (browser, migration, project).
Reviewers: jarek, dsagal
Reviewed By: jarek, dsagal
Subscribers: dsagal
Differential Revision: https://phab.getgrist.com/D3669
Summary:
Adding type check in the PHONE_FORMAT function. Default
conversion to string doesn't work well for floats.
Test Plan: Updated
Reviewers: dsagal
Reviewed By: dsagal
Subscribers: dsagal
Differential Revision: https://phab.getgrist.com/D3701
Summary: Adding support for the "$" syntax in ACL rules.
Test Plan: Updated
Reviewers: georgegevoian, dsagal
Reviewed By: georgegevoian, dsagal
Differential Revision: https://phab.getgrist.com/D3692
We already filter out a line will only None values, and sometimes
Excel of LibreOffice mistakes the real number of columns adding
one or more that have no value at all.
Summary:
Upgrading the friendly-traceback package to include a fix that I specifically requested in https://github.com/friendly-traceback/friendly-traceback/issues/144 as a solution for the problem mentioned in https://grist.quip.com/HoSmAlvFax0j#MbTADAEcJb7 . Specifically, this shows a friendly explanation when using `len()` with a generator expression.
Also upgraded the dependencies `executing` and `stack_data` (which are mine) while I'm at it, although I don't expect this to really change anything.
Test Plan:
Existing tests. There was one test failure because of a new explanation about generic `Exception`s which I've suppressed.
Tested manually that the new explanation appears:
{F64605}
Reviewers: dsagal
Reviewed By: dsagal
Differential Revision: https://phab.getgrist.com/D3687
Summary: Math functions like SUM which call `_chain` were catching `TypeError`s raised by the iterable arguments themselves, e.g. `SUM(r.A / r.B for r in $group)` where `r.A / r.B` raises a `TypeError` would silently return wrong results. This diff narrows the `try/catch` to only check whether the argument is iterable as intended, but not catch errors from the process of iterating.
Test Plan: Added Python unit test.
Reviewers: jarek
Reviewed By: jarek
Differential Revision: https://phab.getgrist.com/D3679
Summary:
Ensure that `lookupOne` (via `RecordSet.get_one`) pays attention to the `sort_by` parameter by picking the first of its sorted list of row IDs.
Allow specifying reverse sort order in `sort_by` by adding `"-"` before the column ID.
Suggested in https://grist.slack.com/archives/C0234CPPXPA/p1665756041063079
Test Plan: Extended Python lookup test
Reviewers: jarek
Reviewed By: jarek
Differential Revision: https://phab.getgrist.com/D3675
Summary:
First iteration for comments system for Grist.
- Comments are stored in a generic metatable `_grist_Cells`
- Each comment is connected to a particular cell (hence the generic name of the table)
- Access level works naturally for records stored in this table
-- User can add/read comments for cells he can see
-- User can't update/remove comments that he doesn't own, but he can delete them by removing cells (rows/columns)
-- Anonymous users can't see comments at all.
- Each comment can have replies (but replies can't have more replies)
Comments are hidden by default, they can be enabled by COMMENTS=true env variable.
Some things for follow-up
- Avatars, currently the user's profile image is not shown or retrieved from the server
- Virtual rendering for comments list in creator panel. Currently, there is a limit of 200 comments.
Test Plan: New and existing tests
Reviewers: georgegevoian, paulfitz
Reviewed By: georgegevoian
Subscribers: paulfitz
Differential Revision: https://phab.getgrist.com/D3509
Summary:
There is a new column in users table called ref (user reference).
It holds user's unique reference number that can be used for features
that require some kind of ownership logic (like comments).
Test Plan: Updated tests
Reviewers: georgegevoian, paulfitz
Reviewed By: georgegevoian, paulfitz
Differential Revision: https://phab.getgrist.com/D3641
Summary:
Adds a "Duplicate Table" menu option to the tables listed on
the Raw Data page. Clicking it opens a dialog that allows you to
make a copy of the table (with or without its data).
Test Plan: Python, server, and browser tests.
Reviewers: jarek, paulfitz
Reviewed By: jarek, paulfitz
Subscribers: jarek
Differential Revision: https://phab.getgrist.com/D3619
Summary:
This diff adds a preview of the value of certain autocomplete suggestions, especially of the form `$foo.bar` or `user.email`. The main initial motivation was to show the difference between `$Ref` and `$Ref.DisplayCol`, but the feature is more general.
The client now sends the row ID of the row being edited (along with the table and column IDs which were already sent) to the server to fetch autocomplete suggestions. The returned suggestions are now tuples `(suggestion, example_value)` where `example_value` is a string or null. The example value is simply obtained by evaluating (in a controlled way) the suggestion in the context of the given record and the current user. The string representation is similar to the standard `repr` but dates and datetimes are formatted, and the whole thing is truncated for efficiency.
The example values are shown in the autocomplete popup separated from the actual suggestion by a number of spaces calculated to:
1. Clearly separate the suggestion from the values
2. Left-align the example values in most cases
3. Avoid having so much space such that connecting suggestions and values becomes visually difficult.
The tokenization of the row is then tweaked to show the example in light grey to deemphasise it.
Main discussion where the above was decided: https://grist.slack.com/archives/CDHABLZJT/p1661795588100009
The diff also includes various other small improvements and fixes:
- The autocomplete popup is much wider to make room for long suggestions, particularly lookups, as pointed out in https://phab.getgrist.com/D3580#inline-41007. The wide popup is the reason a fancy solution was needed to position the example values. I didn't see a way to dynamically resize the popup based on suggestions, and it didn't seem like a good idea to try.
- The `grist` and `python` labels previously shown on the right are removed. They were not helpful (https://grist.slack.com/archives/CDHABLZJT/p1659697086155179) and would get in the way of the example values.
- Fixed a bug in our custom tokenization that caused function arguments to be weirdly truncated in the middle: https://grist.slack.com/archives/CDHABLZJT/p1661956353699169?thread_ts=1661953258.342739&cid=CDHABLZJT and https://grist.slack.com/archives/C069RUP71/p1659696778991339
- Hide suggestions involving helper columns like `$gristHelper_Display` or `Table.lookupRecords(gristHelper_Display=` (https://grist.slack.com/archives/CDHABLZJT/p1661953258342739). The former has been around for a while and seems to be a mistake. The fix is simply to use `is_visible_column` instead of `is_user_column`. Since the latter is not used anywhere else, and using it in the first place seems like a mistake more than anything else, I've also removed the function to prevent similar mistakes in the future.
- Don't suggest private columns as lookup arguments: https://grist.slack.com/archives/CDHABLZJT/p1662133416652499?thread_ts=1661795588.100009&cid=CDHABLZJT
- Only fetch fresh suggestions specifically after typing `lookupRecords(` or `lookupOne(` rather than just `(`, as this would needlessly hide function suggestions which could still be useful to see the arguments. However this only makes a difference when there are still multiple matching suggestions, otherwise Ace hides them anyway.
Test Plan: Extended and updated several Python and browser tests.
Reviewers: paulfitz
Reviewed By: paulfitz
Differential Revision: https://phab.getgrist.com/D3611
Summary:
This diff adds a new `BulkAddOrUpdateRecord` user action which is what is sounds like:
- A bulk version of the existing `AddOrUpdateRecord` action.
- Much more efficient for operating on many records than applying many individual actions.
- Column values are specified as maps from `colId` to arrays of values as usual.
- Produces bulk versions of `AddRecord` and `UpdateRecord` actions instead of many individual actions.
Examples of users wanting to use something like `AddOrUpdateRecord` with large numbers of records:
- https://grist.slack.com/archives/C0234CPPXPA/p1651789710290879
- https://grist.slack.com/archives/C0234CPPXPA/p1660743493480119
- https://grist.slack.com/archives/C0234CPPXPA/p1660333148491559
- https://grist.slack.com/archives/C0234CPPXPA/p1663069291726159
I tested what made many `AddOrUpdateRecord` actions slow in the first place. It was almost entirely due to producing many individual `AddRecord` user actions. About half of that time was for processing the resulting `AddRecord` doc actions. Lookups and updates were not a problem. With these changes, the slowness is gone.
The Python user action implementation is more complex but there are no surprises. The JS API now groups `records` based on the keys of `require` and `fields` so that `BulkAddOrUpdateRecord` can be applied to each group.
Test Plan: Update and extend Python and DocApi tests.
Reviewers: jarek, paulfitz
Reviewed By: jarek, paulfitz
Subscribers: jarek
Differential Revision: https://phab.getgrist.com/D3642
Summary:
Fixes a bug noted here: https://grist.slack.com/archives/C069RUP71/p1662564341132349
This bug could happen quite easily as follows:
1. Have a formula in a summary table such as `$group.amount`. Typically there's also a `SUM` but that's not essential.
2. Find a group with nonzero values of `amount`.
3. Delete all rows in that group in the source table. Typically that just means one row in a lonely group.
4. The summary table row is automatically deleted.
5. Try to undo. This raises an error about trying to update a non-existent summary table row.
I tried to account for this undo problem in https://phab.getgrist.com/D3489 by not saving the updated value for `$group` when it was found to be empty. The reason this was insufficient is that `$group.amount` is immediately invalidated anyway when the source row(s) are deleted (I think because that's just how dependency relations involving references work) *and* the calculated value of `$group.amount` changes even if `$group` doesn't. For example, `$group.amount` may have previously been `[100, 200]`. After deleting the rows, `$group.amount` becomes `[0, 0]`. Keeping `$group` unchanged prevents `$group.amount` from just being `[]`, but deleting the source rows means that the amounts become the numeric default `0` which is still a change. This change in value is then noted which leads to saving an undo action to update the summary table record. All this happens in step 3 above, and the summary record is only deleted after that point.
This diff removes that special handling for `group` and instead adds a more general fix to `action_summary.py`. This inserts undo actions for deleted rows at the beginning of the undo list rather than at the end, which was already done for deleted tables and columns.
Test Plan: Python tests
Reviewers: dsagal
Reviewed By: dsagal
Subscribers: dsagal
Differential Revision: https://phab.getgrist.com/D3626
Eg. before this commit, this table would result in Date columns:
| A | B |
| ----- | -- |
| FALSE | 0 |
For now, even FALSE is parsed as Numeric (not sure why we don't have
a BooleanConverter).
Summary:
This applies the set of dependabot suggestions that are currently
passing tests on grist-core. There are a lot more suggestions to
come, an unusual number are not passing tests because tests were
briefly broken.
The list of suggestions is extracted from:
https://api.github.com/repos/gristlabs/grist-core/pulls?search=status:success+state:open
And then applied using:
yarn upgrade package1@version1 package2@version2 ....
After application, any new entries in package.json are pruned, leaving
just updated entries and yarn.lock changes.
Non-trivial code updates include:
* A change related to axios typing
* A change related to jquery dropping `size()` in favor of `length`
Test Plan: existing tests should pass
Reviewers: jarek
Reviewed By: jarek
Subscribers: jarek
Differential Revision: https://phab.getgrist.com/D3621
Summary:
Undo often leads to errors, especially with summary tables. One example is here: https://grist.slack.com/archives/C069RUP71/p1662564341132349
This diff simply decorates all relevant tests in 3 files testing summary tables with `@test_engine.test_undo`. This didn't catch any new bugs or reveal the problem in the thread above, but it seems good to have.
Test Plan: this
Reviewers: jarek
Reviewed By: jarek
Differential Revision: https://phab.getgrist.com/D3624
Summary:
Python 2 only needs to be supported for the sake of old documents and formulas. This doesn't apply to the separate sandboxes that parse files for imports. Using Python 3 only allows using newer libraries and library versions. In particular, the latest version of openpyxl doesn't support Python 2. This will also make it easier to make other similar changes in the future, such as replacing messytables with a modern library. See https://grist.slack.com/archives/C0234CPPXPA/p1661261829343999?thread_ts=1661260442.837959&cid=C0234CPPXPA
The latest openpyxl is better at handling a particular edge case with broken dates in Excel, but still doesn't quite do what we want, so we monkeypatch it. Discussion: https://grist.slack.com/archives/C02EGJ1FUCV/p1661440851911869?thread_ts=1661154219.515549&cid=C02EGJ1FUCV
Setting `preferredPythonVersion` to '3' in SafePythonComponent ensures that JS always creates import sandboxes that use Python 3. Within Python, a module used by all imports will raise an error in Python 2. Python unit tests of imports are now only run in Python 3, using the `load_tests` protocol of `unittest`.
Test Plan: Mostly existing tests. Added another strange date to the Excel fixture.
Reviewers: dsagal
Reviewed By: dsagal
Subscribers: dsagal
Differential Revision: https://phab.getgrist.com/D3606
Summary:
Based on https://github.com/gristlabs/grist-core/pull/251. It may not look like it, but there's very little going on in this diff:
- Tweak the DATEVALUE doctest for Python 2/3 compatibility.
- Mirrors the PR's changes to requirements3.txt in requirements.txt, i.e. make the same dependency upgrades in Python 2.
- Make the same upgrades in the thirdparty folder for the Python 2 nacl sandbox.
Test Plan: Updated one doctest for dateutil. Checked changelog of sortedcontainers. html5lib is only used by messytables and isn't actually relevant.
Reviewers: paulfitz
Reviewed By: paulfitz
Differential Revision: https://phab.getgrist.com/D3609
Summary:
Raise an exception with a customised message for two cases when a user tries on operation directly on a table without `.all`:
1. For `Table.Col`, where `Col` is an existing column, suggest `Table.all.Col`. If `Col` doesn't exist as a column, fall back to the standard AttributeError.
2. When iterating directly over a table, e.g. `[r for r in Table]`, suggest looping over `Table.all` instead.
Test Plan: Added Python unit tests.
Reviewers: georgegevoian
Reviewed By: georgegevoian
Differential Revision: https://phab.getgrist.com/D3593
Summary:
Makes the following improvements to formula autocomplete:
- When a user types `$RefCol` (or part of it), also show `$RefCol.VisibleCol` (replace actual column names) in the autocomplete even before the `.` is typed, to help users understand the difference between a raw reference/record and its visible column.
- When a user types a table name, show `.lookupOne` and `.lookupRecords` in the autocomplete, again even before the `.` is typed.
- For `.lookupRecords(` and `.lookupOne(`, once the `(` is entered, suggest each column name as a keyword argument.
- Also suggest lookup arguments involving compatible reference columns, especially 'reverse reference' lookups like `refcol=$id` which are very common and difficult for users.
- To support these features, the Ace editor autocomplete needs some patching to fetch fresh autocomplete options after typing `.` or `(`. This also improves unrelated behaviour that wasn't great before when one column name is contained in another. See the first added browser test.
Discussions:
- https://grist.slack.com/archives/CDHABLZJT/p1659707068383179
- https://grist.quip.com/HoSmAlvFax0j#MbTADAH5kgG
- https://grist.quip.com/HoSmAlvFax0j/Formula-Improvements#temp:C:MbT3649fe964a184e8dada9bbebb
Test Plan: Added Python and nbrowser tests.
Reviewers: paulfitz
Reviewed By: paulfitz
Differential Revision: https://phab.getgrist.com/D3580
Summary: Extend formula error messages with explanations from https://github.com/friendly-traceback/friendly-traceback. Only for Python 3.
Test Plan: Updated several Python tests. In general, these require separate branches for Python 2 and 3.
Reviewers: georgegevoian
Reviewed By: georgegevoian
Differential Revision: https://phab.getgrist.com/D3542
Summary: Using the `random` module in the Grist `UUID()` function is not cryptographically secure, and is only necessary for the old pynbox (Python 2) sandbox which doesn't support `os.urandom`. This diff uses the `uuid.uuidv4()` function from the Python standard library when possible, which is more secure, only falling back to the old implementation when necessary.
Test Plan: Added Python unit tests to check both implementations.
Reviewers: dsagal
Subscribers: paulfitz, dsagal
Differential Revision: https://phab.getgrist.com/D3578
Summary:
Formulas in summary tables were being associated with the source table for automatic updating. When a table/column was renamed such that the formula needed to update to match, it would look for a column with the same colId but in the source table. Such a column might not exist which would lead to an error, or if it existed then the update would be wrong.
This association was created while building formulas to display in the code view in a nested `_Summary` class, it didn't need to exist at all. So this diff simply prevents the association from being created.
User report and discussion: https://grist.slack.com/archives/C0234CPPXPA/p1659717322297019
Test Plan: Extended `TestSummary.test_table_rename` Python test.
Reviewers: georgegevoian
Reviewed By: georgegevoian
Differential Revision: https://phab.getgrist.com/D3568
Summary:
Conditional formatting can now be used for whole rows.
Related fix:
- Font styles weren't applicable for summary columns.
- Checkbox and slider weren't using colors properly
Test Plan: Existing and new tests
Reviewers: paulfitz, georgegevoian
Reviewed By: georgegevoian
Differential Revision: https://phab.getgrist.com/D3547
Summary:
The MIN and MAX functions for formulas previously only considered numbers, ignoring other types, including dates. An example of this being a problem is here: https://community.getgrist.com/t/last-field-circularreferror-what-is-it/1114/4 . Using `MIN` on a column of dates would return 0 (the default) which gets converted to 1970-01-01. Users have to use `min` instead, which is confusing, and doesn't work when some values are empty.
This diff lets the functions operate on date and datetime values. A mixture of dates and datetimes is allowed, even though these cannot usually be compared in Python. Mixing dates and numbers will raise an exception.
Test Plan: Extended doctests
Reviewers: jarek, paulfitz
Reviewed By: jarek
Subscribers: paulfitz
Differential Revision: https://phab.getgrist.com/D3560
Summary: Displays a live row count of each table on the Raw Data page.
Test Plan: Browser tests.
Reviewers: alexmojaki
Reviewed By: alexmojaki
Differential Revision: https://phab.getgrist.com/D3540
Summary:
This calls a new `initialize` method on the sandbox before we start
doing calculations with it, to make sure that `random.seed()` has
been called. Otherwise, if the sandbox is cloned from a checkpoint,
the seed will have been reset.
The `initialize` method includes the functionality previously done
by `set_doc_url` since it is also initialization/personalization and
this way we avoid introducing another round trip to the sandbox.
Test Plan: tested with grist-core configured to use gvisor
Reviewers: georgegevoian, dsagal
Reviewed By: georgegevoian, dsagal
Subscribers: alexmojaki
Differential Revision: https://phab.getgrist.com/D3549
Summary:
Comprehensions iterating over `Table.all` like `[foo.bar for foo in Table.all]` led to an error when renaming the column `bar`. This diff fixes that so that renaming `bar` does the same thing as for a comprehension over `Table.lookupRecords()`. Note that `next(foo for foo in Table.all).bar` is still not supported, as the same is not supported for `Table.lookupRecords()` either.
Discussion: https://grist.slack.com/archives/C069RUP71/p1658360276762949
Test Plan: Parametrised existing Python test to test the same thing for both `all` and `lookupRecords`
Reviewers: dsagal
Reviewed By: dsagal
Subscribers: dsagal
Differential Revision: https://phab.getgrist.com/D3538
