cybergis / cybergis-compute-core

Apache License 2.0
7 stars 6 forks source link

Documentation overhaul, linting, and file transfer optimizations #109

Closed yianzhang14 closed 4 weeks ago

yianzhang14 commented 4 months ago
yianzhang14 commented 3 months ago

Fixed some remaining bugs and did more rigorous testing:

also fixed the github actions to copy config jsons and to actually lint things

yianzhang14 commented 3 months ago
yianzhang14 commented 1 month ago

Is it possible to split this PR between the linting/doc changes and your actual code changes? Or at least could you let me know which files have actual changes to the functionality vs. just linting?

I could try to separate the PR out, but I think that would take a considerable amount of time since everything is kinda intertwined at this point. However, I think all the changes can be boiled down into four categories:

  1. Pure linting changes - these come from ESLint & are largely stylistic/have little semantic changes on what the code does (aside from stuff like changing vars to consts, which required some restructuring of things, especially in for loops--iterateindices to values w/ for-each loops). This also includes various comments/docstrings I added throughout the codebase and me changing all ==/!= to more safe === !== (which could break things if the unsafeness was intended). As a result, these changes largely should be non-breaking.
  2. Typescript compilation changes - these come from setting strict: true in the tsconfig.json file. Resolving these took a bit more effort/changes beyond just code style. I think the main problems I had to fix from this involved getting rid of all Anys, adding explicit typing everywhere I could, moving state up and down the inheritance hierarchy, changing some interface type signatures from ?: to : (i.e., force them to not be undefined/null), and adding explicit assertions that things aren't null/undefined when we expect them not to be. These changes also should be largely non-breaking, since they only affect/relate to the compilation of things, and not their actual runtime behavior.
  3. Library upgrades/changes - these come from updating libraries to newer versions and removing unneeded libraries. For the former, this largely involves updating typeorm and redis and adding the isomorphic-git sdk. With typeorm 0.3, the API underwent a significant change, so DB.ts was altered significantly, and through this, all database interactions had their syntax changed (but this was just a direct translation of the previous things). With the new redis versions, things just became a lot easier to use, and I essentially just took all the scattered redis classes defined throughout and centralized them into an inheritance hierarchy within Redis.ts. With isomorphic-git, I replaced most of the execs in GitUtil.ts. For the removing packages part, this was relatively straightfoward as these packages were not used anywhere or were used sparsely--e.g., bodyparser for express, which was originally a one-liner middleware for parsing request jsons but which is now deprecated (native express can do this now). These also should be relatively non-breaking, since they only involve translating old API calls to new ones, although there could've been a transcription error.
  4. Actual rewrites - these actually change the logic of the code and either implement new features/make convoluted things more streamlined. The two major sources of this includes GitUtil.ts and FolderUploader.ts, which were changed to allow for manifest caching and server-side git repo caching. These changes have the highest likelihood of being breaking.
yianzhang14 commented 1 month ago

As a brief outline of which files fall into what category:

yianzhang14 commented 1 month ago

I'll start working on these in the next few days. For the second issue, I think that may be due to me changing some database schemas to comply with the typescript compiler. I'll try to revert those with some exception comment/flag--they weren't exactly critical.

alexandermichels commented 1 month ago

Sounds good. You don't necessarily have to revert the database changes if you have a simple solution for updating an existing database to fit the new schema. So for example, a script that can update the data types/whatever needs to be done. Just need a way to push these changes to dev for testing and eventually production. Great work so far!

alexandermichels commented 1 month ago

@yianzhang14 we are trying to include this in next month's release, so we need this sorted out soon and deployed on our test server. When do you think you can get this working with instructions for deploying to test/production?

yianzhang14 commented 1 month ago

Sorry, just recently got back to the US but I should have some time this week to sort it out. Can you let me know the exact server you're trying to deploy to?

alexandermichels commented 1 month ago

No worries. The exact server is less important than having the documented process for deploying it. We will have to deploy this to test, then production and since this is OSS we should have documentation to deploy anywhere.

You can try on test and keep track of any changes/code you run. I'll use that documentation to deploy on production.

yianzhang14 commented 1 month ago

I just fixed the merge conflicts, but I can't seem to recreate the error you received with typeorm. The errors it's throwing shouldn't be changing anything, as I only changed some type annotations.

alexandermichels commented 1 month ago

Yeah not sure what happened, maybe it just needed to be run twice to reformat the DB? Everything is working with the new commits on test.

One issue when running the three-examples job: I saw this issue in the logs:

Assertion failed: 'Variable is undefined/null when it should not be. Assertion at Error, SingularityConnector.prepare: 70'
Assertion failed: 'Variable is undefined/null when it should not be. Assertion at Error, SingularityConnector.prepare: 71'

however, the job still ran fine. May just want to look into that to see if its an issue or not. Going to ask some collaborators to test out their more complex jobs on the server now.

yianzhang14 commented 1 month ago

I think I brought that up in some meeting before. I essentially added some sanity checks that print those things if things were null, and that popped up for some user-related things when building the slurm config. It doesn't seem to affect anything from what I've seen, but it should probably be resolved at some point.