Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Database API: generate printers through reflection #615

Open
17 of 21 tasks
lars-t-hansen opened this issue Oct 8, 2024 · 7 comments
Open
17 of 21 tasks

Database API: generate printers through reflection #615

lars-t-hansen opened this issue Oct 8, 2024 · 7 comments
Assignees
Labels
component:sonalyze sonalyze/* task:cleanup Non-functional (or apparently so) code change

Comments

@lars-t-hansen
Copy link
Collaborator

lars-t-hansen commented Oct 8, 2024

This is cleanup I've been working on in the background that's turning out ok, so let's track it: all the "tables" and "reports" exposed by sonalyze have custom, hand-maintained printing code, meaning the code is maintained in several scattered places and it's a bit of a mess to remember how to maintain it when it needs to change. It's possible to use Go's reflection API to instead generate printers from the structure definitions with some annotations; this brings everything together in one place (with the caveat that there can't easily be multiple different printers for the same data). This is not completely free: code that now performs computation as part of printing must be lifted into a separate computation step that computes the result table that is then printed. But the consequence of doing that is a cleaner separation of concerns.

In order not to block the adding of new fields to the sonar output and to avoid merge hell if we do, I want to land what I have asap, and then mop up the remaining cases (jobs and parse) later. To-do:

  • add modifiers /sec and /iso for timestamps? not sure if we need them for current formatters. investigate.
  • pre-review
  • clean up after pre-review
  • refresh the relative test harness in code/tests/relative and implement tests
    • cluster
    • config
    • load
    • metadata
    • node
    • profile
    • sacct
    • uptime
  • implement sensible test cases
  • tests have to run and pass against -data-dir on naic-monitor
  • should vet all the names that they are consistent! Hostname not Host, RequestedCPUS not ...CPUs or Cpus, etc. Might develop a small vocab for this.
  • final review

Then the next step(s):

  • parse
  • jobs
  • handle attributes and modifiers (crucial for both parse and jobs, so will happen)
  • better test cases in the reflection code itself, see comments below
  • simple unit tests for every command via command mocking, requires slight restructuring
@lars-t-hansen lars-t-hansen added task:cleanup Non-functional (or apparently so) code change component:sonalyze sonalyze/* labels Oct 8, 2024
@lars-t-hansen lars-t-hansen self-assigned this Oct 8, 2024
@lars-t-hansen
Copy link
Collaborator Author

(Currently on branch larstha-reflection.)

@lars-t-hansen
Copy link
Collaborator Author

This is a patch of some pending work for parse using the more elaborate formatter specs. Much of this has landed but the changes to parse have not, nor the supporting code in reflect.go.
cfs.patch.txt

@lars-t-hansen
Copy link
Collaborator Author

It could look like /sec is a modifier only used by jobs at the moment, so we don't need to implement that for this iteration.

@lars-t-hansen
Copy link
Collaborator Author

Incompatibility that is not yet fixed: the reflective printer insists on printing boolean values as 0/1 but the old printer sometimes uses no/yes (xnode for config, gpumempct for config + node), other times 0/1 (gpu_status for jobs, which is internally not a bool but is a bool sort of in printed data, it prints "0" for ok and "1" for failure, so it really is the GpuFail field). The documentation for "nodes" actually insists that we print "yes" or "no". I'm inclined to rethink what the reflective printer does. If we always print "yes" and "no" for true and false, and redefine the gpu_status field as an integer (bit vector maybe, or a number) then we remain compatible. There are lots of hacks in the tests for node and config to work around this problem, they would disappear too.

@lars-t-hansen
Copy link
Collaborator Author

The profile tests (like the cluster tests, really) will require fixes to both the old and new code, or we kick it down the road: unless processes in a job are completely sorted the profiles will be completely uncomparable. This should happen anyway (#600) and not just for the sake of testing, but for now it's a headache.

@lars-t-hansen
Copy link
Collaborator Author

Developing the vocabulary in sonalyze/VOCAB.md

@lars-t-hansen
Copy link
Collaborator Author

lars-t-hansen commented Oct 29, 2024

Note this comment on the patch for future work:

Ideally:

  • test all the types we handle specially
  • test that string slices are sorted properly (and the input slice is unaffected)

At the moment, the relative test cases we run manually are good enough, but are not going to be run forever, so consider moving basic testing in here.

lars-t-hansen added a commit that referenced this issue Oct 29, 2024
For #615 - Implement formatting by reflection for most verbs
lars-t-hansen added a commit that referenced this issue Nov 4, 2024
lars-t-hansen added a commit that referenced this issue Nov 11, 2024
For #615 - Documentation and test changes uplifted from jobs branch
lars-t-hansen pushed a commit that referenced this issue Nov 12, 2024
lars-t-hansen pushed a commit that referenced this issue Nov 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component:sonalyze sonalyze/* task:cleanup Non-functional (or apparently so) code change
Projects
None yet
Development

No branches or pull requests

1 participant