Migrate from Moment to Luxon

[jamescrowley]

Closes #408

Types of changes

• Chore (non-breaking change which does not add functionality) (added as none of the others fitted)
• Enhancement (project structure, spelling, grammar, formatting)
• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)
• My code follows the code style of this project.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have added tests to cover my changes.
• All new and existing tests passed.

A description of the changes proposed in the Pull Request

This PR removes the moment and moment-range dependency, and replaces the implementation with luxon. This is mostly a simple translation with a few notes:

• the behaviour of the current implementation when timestamps are invalid, or are supplied in a non-UTC timezone was not tested. I added tests in order to capture the existing behaviour, and preserve it.
• momentRange.range and fromDateTimes are both "Inclusive of the start but not the end.". However, when operating on these date time intervals, 'by' (moment) includes the end, but 'splitBy' (luxon) does not. Code has been adjusted for this.
• extracted one method for padding per second within a date range as a follow-up refactor.

Some items to discuss:

• To maintain current behaviour, this PR converts to UTC prior to outputting in ISO format, regardless of the timezone/offsets of the input timestamps.
• the behaviour of the existing and proposed implementation when timestamps are invalid could do with review.
• if the refactor is ok in this PR or should be a separate follow up (or not at all)

Let me know any thoughts/concerns.

[jmcook1186]

thanks for doing this so fast - we'll get it reviewed asap!

[jmcook1186]

Hi - thanks for this - swapping out the libraries look good, we've ended up making some changes to the IF on dev and now there are a couple of conflicts to resolve.

However, when we checkout this branch and run the example/time-sync.yml manifest file we get empty output arrays. We'll want to see if that behaviour persists after the conflicts are resolved and investigate why this is happening.

[jamescrowley]

@jmcook1186 hmm, strange. I get

{"name":"nesting-demo","description":"impl with 2 levels of nesting with non-uniform timing of observations","tags":null,"initialize":{"models":[{"name":"teads-curve","path":"@grnsft/if-unofficial-models","model":"TeadsCurveModel"},{"name":"sci-e","path":"@grnsft/if-models","model":"SciEModel"},{"name":"sci-m","path":"@grnsft/if-models","model":"SciMModel"},{"name":"sci-o","path":"@grnsft/if-models","model":"SciOModel"},{"name":"time-synchronization","path":"builtin","model":"TimeSyncModel","config":{"start-time":"2023-12-12T00:00:00.000Z","end-time":"2023-12-12T00:01:00.000Z","interval":5}}]},"graph":{"children":{"child":{"pipeline":["teads-curve","sci-e","sci-m","sci-o","time-synchronization"],"config":{"teads-curve":{"thermal-design-power":65},"sci-m":{"total-embodied-emissions":251000,"time-reserved":3600,"expected-lifespan":126144000,"resources-reserved":1,"total-resources":1},"sci-o":{"grid-carbon-intensity":457}},"children":{"child-1":{"inputs":[{"timestamp":"2023-12-12T00:00:00.000Z","duration":10,"cpu-util":10,"carbon":100,"energy":100,"requests":300},{"timestamp":"2023-12-12T00:00:15.000Z","duration":10,"cpu-util":20,"requests":300},{"timestamp":"2023-12-12T00:00:35.000Z","duration":15,"cpu-util":20,"requests":400}],"outputs":[]},"child-2":{"inputs":[{"timestamp":"2023-12-12T00:00:00.000Z","duration":10,"cpu-util":10,"requests":300},{"timestamp":"2023-12-12T00:00:10.000Z","duration":30,"cpu-util":20,"requests":380},{"timestamp":"2023-12-12T00:00:50.000Z","duration":20,"cpu-util":20,"requests":380}],"outputs":[]}}}}}}

running

npm install "@grnsft/if-models"
npm install "@grnsft/if-unofficial-models"
npm run impact-engine -- --impl examples/impls/test/time-sync.yml 

I assume there's no automated test for this? All the unit tests pass too. Is there a way to trigger the PR in CI/CD?

I'll resolve the conflicts now anyway and then we can see what's going on.

[jamescrowley]

I've now rebased @jmcook1186 and now I get the same behaviour as you.

There's some strange interplay between whether padding is necessary, and date formatting being passed in for start-time - all the tests use ISO format (which is why they pass), but when loaded from YAML it comes as Tue Dec 12 2023 00:00:00 GMT+0000 (Coordinated Universal Time). I'd already noticed that dates in unexpected formats have pretty undefined behaviour - so definitely tempted to add some further validation here.

I don't quite get all the interplay yet as to why it worked based off 477be28 but doesn't now. I'll have a further dig when I get a moment.

[jmcook1186]

Hi again - I found some time to look into this and push some fixes - my branch is now processing the manifest file correctly and passing most (not yet all) of the unit tests. I didn't quite get time to finish this off - hoping to get back to it on Monday.

My branch is here for reference https://github.com/Green-Software-Foundation/if/tree/luxon-fixes

Planning to finish this off on Monday morning, but happy to back off if you want to work on it more. If I don't hear otherwise I'll PR into your branch, and then you can review it, add anything else you like and PR from your branch into if/dev.

[jamescrowley]

@jmcook1186 I’m confused - are you saying tests are failing on this branch on your machine? They all passed for me both before & after the rebase, the issues with the manual integration test not withstanding. Is there some other suite of tests not picked up by just ‘npm run test’?

Sorry for the runaround, definitely didn't intend to submit something that you're then having to fix up just to get tests passing! Is there a plan to get CI/CD up and running on this repo? I could look to set something up if useful. At least as a new contributor it would have been helpful to have that to be sure my code was at least passing all the expected tests in a controlled environment?

[jmcook1186]

oh.. sorry if i've interfered where it wasn't necessary! I may have misunderstood the status of the PR.

we basically just need to see that the plugin works when invoked from a manifest file, i.e.

npm run impact-engine -- --impl examples/impls/test/time-sync.yml

should generate the expected output data, while also all the unit tests pass.

When I checked out the PR I could get the unit tests passing but not the integration. Then when I got the integration to work, the unit tests all failed.

We don't have formal tests outside npm run test - we just manually check that the manifest files execute correctly. We should have some CI/CD in place this week.

[jamescrowley]

@jmcook1186 gotcha and definitely not interference on your part 😀 just wanted to be clear where I’d gone awry! I’ll take a look at your commits and getting the integration test passing tomorrow and see how it goes, unless it’s blocking other changes in which case feel free to push ahead (end of day here as I’m in Sydney)

[jamescrowley]

@jmcook1186 the issues stem from how dates are passed into time-sync. The tests pass strings, in ISO format. When running from the CLI and parsing YAML, we get a JS Date object (when it's a valid date - but it might be a string if the YAML parser doesn't identify it as a date)

For now I've made this handle both ISO strings and date objects. Please see the last two commits.

• What is the expectation re date formats being fed to time-sync.ts? We may want to consider turning off the attempted Date parsing of the YAML library, and then explicitly parse based on our own schema rules? It also feels like time-sync.ts should not be having to deal with potentially invalid, missing and different data types for dates at this stage - instead, perhaps we pre-process the data to convert to appropriate date objects and spit out errors beforehand.
• I was also tempted to re-introduce a strongly-typed definition for timestamp and duration in the ModelParams as they seem mandatory to the core schema, but not sure on the direction you folks are heading with that, so I've held off to avoid blowing out the scope of this PR.

[jmcook1186]

Hi - awesome, thank you!

The expectation is that timestamps will be passed in as ISO 8061 strings. This should always be the case, so yes, I agree we can validate this outside of the time-sync model.

Let's hold off on altering the ModelParams definition for now, as we are doing some substantial refactoring int he background and will likely handle this as part of that.

[jamescrowley]

@jmcook1186 ok 👍 Just let me know if you need/want anything else on this PR. If you like, I can follow up with a separate PR to consolidate the validation and possibly disable the auto parsing done by the YAML library so we can parse to a tighter spec.

[jmcook1186]

This looks great - all tests passing and manifest files being processed as expected.
Thanks a lot for your work on this - we appreciate it!
I added a commit to resolve merge conflicts in package.json.