Updated storage and training files to enable text generation tasks. A… #640
Conversation
…dditionally, made minor updates to the environment and ensured everything passes the standard Python compliance tests, maintaining compatibility with Modyn's existing functionality.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #640 +/- ##
==========================================
- Coverage 85.58% 81.72% -3.86%
==========================================
Files 258 261 +3
Lines 11377 12089 +712
==========================================
+ Hits 9737 9880 +143
- Misses 1640 2209 +569 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
I briefly skimmed the PR. Thanks for the work! The biggest feedback right now would be that we should not have parallel codepaths which are mostly copy/paste of existing ones but without labels. Rather, let's have clean code where we move labels to be optional, both in the c++/protobuf/python worlds. And before I do the proper review, would be great if you could check the diff for changes that should not go onto main :)
Thanks!
| @@ -30,7 +30,7 @@ dependencies: | |||
| - psycopg2 | |||
| - sqlalchemy>=2.0 | |||
| - pyaml | |||
| - pydantic | |||
| - pydantic==2.9.2 | |||
There was a problem hiding this comment.
This should be solved with the latest commit on main
| # - nvidia::cuda-libraries-dev=12.1.* | ||
| # - nvidia::cuda-nvcc=12.1.* | ||
| # - nvidia::cuda-nvtx=12.1.* | ||
| # - nvidia::cuda-cupti=12.1.* | ||
| # - nvidia::cuda-cudart-dev=12.1.* | ||
| # - nvidia::cuda-profiler-api=12.1.* | ||
| - pytorch::pytorch-cuda=12.1 | ||
| - nvidia::cuda-libraries-dev=12.1.* | ||
| - nvidia::cuda-nvcc=12.1.* | ||
| - nvidia::cuda-nvtx=12.1.* | ||
| - nvidia::cuda-cupti=12.1.* | ||
| - nvidia::cuda-cudart-dev=12.1.* | ||
| - nvidia::cuda-profiler-api==12.1.* |
There was a problem hiding this comment.
should be commented out. no changes here should be necessary
| ignore_existing_trigger_samples: false | ||
| ignore_existing_trigger_samples: true |
| @@ -255,7 +255,7 @@ class SelectorConfig(HostnamePortMixin): | |||
| ), | |||
| ) | |||
| ignore_existing_trigger_samples: bool = Field( | |||
| False, | |||
There was a problem hiding this comment.
also shouldnt merge that
| @@ -0,0 +1,365 @@ | |||
| #plimport pytorch_lightning as pl | |||
There was a problem hiding this comment.
this file needs to be cleaned before merging it. remove all unnecessary comments. there should only be necessary functions, e.g. there should not need to be the need for evaluation code in here
| @@ -0,0 +1,152 @@ | |||
| //"""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""" | |||
| Status Get(ServerContext* context, const modyn::storage::GetRequest* request, | ||
| ServerWriter<modyn::storage::GetResponse>* writer) override; | ||
| Status GetNL(ServerContext* context, const modyn::storage::GetRequest* request, |
There was a problem hiding this comment.
similar to my comment on the proto file, let's not introduce second codepaths as much as possible, because this seems to replicate a lot of logic. rather, let's add labels if we have them in the orgiinal code path
| @@ -74,6 +74,8 @@ class StorageServiceImpl final : public modyn::storage::Storage::Service { | |||
|
|
|||
| Status Get(ServerContext* context, const modyn::storage::GetRequest* request, | |||
| ServerWriter<modyn::storage::GetResponse>* writer) override; | |||
| Status GetNL(ServerContext* context, const modyn::storage::GetRequest* request, | |||
| ServerWriter<modyn::storage::GetResponseNoLabels>* writer) override; | |||
There was a problem hiding this comment.
also i think this file in general needs to be cleaned
| @@ -409,8 +626,45 @@ def _fetch_partition_noprefetch( | |||
| container["weights"][idx], | |||
| ) | |||
|
|
|||
| def _fetch_partition_noprefetch_generative( # based on the one above | |||
There was a problem hiding this comment.
similar comments here. I would propose to refactor the PR such that we don't have parallel codepaths for generative
| @@ -57,7 +57,8 @@ def __init__( | |||
|
|
|||
| self.shuffle = request.shuffle | |||
| self.enable_accurate_gpu_measurements = request.enable_accurate_gpu_measurements | |||
|
|
|||
| self.generative=request.generative | |||
| print(f"generetive:{self.generative}") | |||
Made minor updates to the environment and ensured everything passes the standard Python compliance tests, maintaining compatibility with Modyn's existing functionality. Added GPT-2 as a model that can be used along with the GPT-2 tokenizer. Introduced the generative flag to the Modyn pipeline, enabling it to distinguish between generative and classification tasks. Also updated some Python test files to incorporate this flag, ensuring no errors are thrown.