Skip to content

Conversation

@Synicix
Copy link
Contributor

@Synicix Synicix commented Nov 1, 2024

Added pod job, InputStoreMapping, Input, StorePointer, and OutputStorePointer.

Update issue #11 to explained design choices found in this pull request

@Synicix Synicix requested review from eywalker and guzman-raphael and removed request for eywalker November 1, 2024 23:44
@Synicix Synicix marked this pull request as ready for review November 1, 2024 23:44
src/model.rs Outdated
pub hash: String,
/// Details about the pod from which the pod job was created from
pub pod: Pod,
input_volume_map: BTreeMap<PathBuf, PathBuf>,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

File hash -> stream key. Bring this up for dicussion tomrrow.
Perhaps need to implement input / store first.

Copy link
Contributor

@guzman-raphael guzman-raphael left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A summary from our discussion yesterday is that your implementation of the current design is great and accurate here, however, I have some doubts about our design.

Mainly, feel that:

  • Input model should be implemented first (or with this PR) so that PodJob can have a reference to it (like how PodJob refers to a Pod).
  • A stream's key doesn't seem to be referenced in PodJob. That way less things would break if there are updates to the associated Pod.

Oh, and please update your linked issues and the project board to reflect the current status of the issues you are working on.

@guzman-raphael guzman-raphael linked an issue Nov 8, 2024 that may be closed by this pull request
@Synicix Synicix marked this pull request as draft November 14, 2024 08:13
@Synicix Synicix marked this pull request as ready for review November 16, 2024 08:41
@Synicix
Copy link
Contributor Author

Synicix commented Nov 16, 2024

Summary of changes:

  • Add pod job model
  • Add save and load functions for store for the file store
  • Add compute_check_sum for file or folder to the store trait and implmented it in filestore with merkle_hash for efficient computation (it uses Blake3 which is apprently faster than sha256, but need to actually indepdent test it)
  • Add wipe functionailty for wiping entire store
  • Change filestore functions to fully support annotationless operations
  • Redesign filestore test to full test all model more parallelization

@Synicix Synicix linked an issue Nov 16, 2024 that may be closed by this pull request
Copy link
Contributor Author

@Synicix Synicix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Old review

src/model.rs Outdated
pub fn get_store(&self) -> Result<impl FileStore> {
// Load the yaml into a Btreemap, pull out the class, then build the store

let storage_class_name = self.uri.split("::").collect::<Vec<&str>>()[0];
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

change it to // instead of ;:

Copy link
Contributor Author

@Synicix Synicix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Notes from meeting

src/store/mod.rs Outdated
}

/// Trait to be implemented by file stores
pub trait FileStore: Sized {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change name to data store

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

src/store/mod.rs Outdated
/// Standard behavior of any store backend supported.
pub trait Store {
/// How a pod is stored.
pub trait ModelStore: Sized {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add filestore as a trait dependency

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixe

tests/model.rs Outdated
Ok(())
}

#[test]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add data store trait tests

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Defer till orchestrator

@Synicix
Copy link
Contributor Author

Synicix commented Dec 17, 2024

Changes made, ready for review @eywalker @guzman-raphael

guzman-raphael and others added 12 commits January 15, 2025 01:18
…d order and update `hash` to be used in more contexts.
…user data in `{store_directory}/orcapod_data`, and update tests to reflect our use-case example from Stanford 11/2024 trip.
…, add pod deserializer based on default, add clone support for models to give greater flexibility in tests, add minimal pod job model, change output of `load_model` to allow caller to control how to set fields, allow new min ident, (for now) add `anyhow` as dev-dependency to get stack trace when debugging, fix bug in `list` call in `basic_test`, and add new `pod_job` tests.
…et backtrace to display by default in DevContainer/GHA, and defer default user data subdirectory creation to orchestrator (i.e. later).
…hecksums, enable `todo!` for some untested cases, add user data for tests that need it, and update tests.
…ve unneeded error handling, rename `find_annotation` -> `find_model_metadata` to account for `spec.yaml` search, simplify, and update tests.
Apply review suggestions (going to edit the tests)
@guzman-raphael guzman-raphael merged commit a44c844 into nauticalab:dev Jan 24, 2025
1 check passed
@guzman-raphael guzman-raphael linked an issue Jan 31, 2025 that may be closed by this pull request
@Synicix Synicix deleted the pod-job2 branch March 17, 2025 23:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Implement thiserror Add InputSource model Add PodJob, Input, InputStoreMapping, and OutputStoreMapping Model

2 participants