🐛 Bug: Ersilia Test Command: False Positive Test Failure #1203
Comments
|
Hi @DhanshreeA . Could you check my logic of the bug for further understanding? After navigating the codebase, I believe that the error is being produced because there are calls to the From your explanation this morning, were you suggesting that you want to do a complete overhaul of the I was also looking into how the |
|
Hi @miquelduranfrigola and @GemmaTuron I created a script to test the different methods of the It seems that the example and random example methods are able to run. I tried creating a test for predefined examples and the output states it does not work, but this may be because the test I created was not correct. Here is the output after running it on my local machine: |
|
Additionally, I am trying to print out the input and output info but I am not seeing it being displayed on the log output? I have pushed the print statements and debugging onto this branch. I also added the return statement in the Log output: |
Hey @kurysauce,
|
Hey @kurysauce good effort on writing a test for the ExampleGenerator class, however there's a major fallacy in your test for checking predefined_example, you're defining ie your overwriting of |
Hey @kurysauce, I think I understand what's going on, let's look at this step by step. We see in the logs that you shared, there's a line that says: Now if you look for this line in the codebase, you'd find it here. This gives us some clues! You'll notice that this line is after the call to ExampleGenerator.example, ie, that part is not what's raising the error, but it's possibly coming from its result. If we inspect this method, we can see that it returns nothing, however we are still trying to capture its return value in The next one, |
|
Running into a raised error in the I might be cd'd into the wrong directory, leading to incorrect relative path resolution. The base directory returned by |
|
the tests: are working for me locally. Can others verify? |
|
Thanks @kurysauce ! |
|
Hi @kurysauce - I'll take a look into this, but could you also update your fork against the latest code in ersilia, there's a slim chance that it might fix the issue? |
Link to Fork https://github.com/kurysauce/ersilia_test/blob/master/ersilia/publish/test.py |
Thanks @DhanshreeA! I synced my fork but the issue still remains. I just had my meeting with Miquel and we discussed that the problem is this line of code: https://github.com/kurysauce/ersilia_test/blob/85f4e9df8ce47ec472ea3bc6d903308cf7eff652/ersilia/publish/test.py#L542C15-L543C57 in the Additionally, we also discussed that the method of calculating the model size is incorrect—the size of the model parameters and the dependencies must be calculated through navigating the path folders. This is what I'm currently working on right now! |
|
Hi @miquelduranfrigola, a quick question. The paths eos/dest/"model_id" and eos/repository/"model_id" , have no subfolders labeled frameworks or checkpoints, like what we discussed yesterday in our meeting. But, the subfolders exist in my local download of the model, just not the eos folder. How should I resolve this discrepancy? I have the output attached on slack. |
|
Mmm... this is interesting. And you fetched the model successfully before testing? |
|
@miquelduranfrigola @DhanshreeA Yes, I was able to fetch and serve the model before testing. From Dhanshree's message on Slack, it seems that the test command tests "a model that has been specified through Should the test command parsing be different then, perhaps navigating the directories starting with |
I went ahead and created an additional path parsing to be specified using the Additionally, I added more logging info of the count and types of the extensions in both the With the modified paths, the
Since the Ersilia run is completed and the next line that is called is ersilia_run = self.read_csv(output_file) I am assuming there is a problem reading in the output file. One thing that I notice is that the
|
|
Hello @kurysauce Quick answer. Indeed, there is a problem with parsing the output file. The first two columns correspond to the molecule InChIKey (column names |
Great thanks @miquelduranfrigola! Should I implement the EOS paths we discussed Monday or stick with the repo_path I currently have implemented? I fetched using the repo path flag, but the code for the EOS path still did not have the subfolders. Since the output path is not being parsed correctly, would it be effective to edit the read_csv function to read from the third column on? I hesitate because this would affect other model outputs being read vs the current one I am testing (eos1pu1). |
|
For updates, I explored hard-coding the The error yields: I verified that the bash_output.csv path is the same as the existing temporary files in the logs. I believe the error is in two places:
I am not sure how to fix the access of the Relevant Log Output: |
I tried fixing the first two lines of the bash script using:
but this did not work ask well. |
|
Updated Logs for empty output errors, logs indicate existing output: |
|
This is looking good, @kurysauce - Tagging @DhanshreeA so she is in the loop |
|
For updates, implemented a potential fix called |
Update: Completed the Updated Log |
Update: I believe I fixed the issue and the update code is pushed to my fork. Can others test? |
|
Thanks @kurysauce! (just a small note, we don't want to parse the last two columns only, we strictly want to parse all columns from the third one (i.e. index 2 in python) to the end of the CSV file. This is what you are doing, so all is good). @DhanshreeA and @HarmonySosa - do you think we can quickly test this? @kurysauce please give instruction on which commands you'd like folks to test. I recommend using Codespaces. Many thanks! Great progress! |
Hi @DhanshreeA and @HarmonySosa ! I agree, running the testing on Codespace works better for me than on my local environment. Fetch the models with the I started testing a few different models based on their output types: eos8a5g - string: passes I am particularly paying attention to the outputs in the |
|
Hey @kurysauce Fantastic work on this so far! I see that the test command has evolved phenomenally and it would be really helpful if we could document all the changes somewhere - especially the following points:
I think for now putting these details in a Google doc would be sufficient, and we'll try to move it to our GitBook within the model contribution template while you're here. |
Here is the link to the Googel Doc Link! I believe I have made all the edits requested as well @DhanshreeA. |
|
Closed with PR |
Describe the bug.
When using the
testcommand for certain models an error message appears indicating a failure with Ersilia. However, the model will appear to produce the desired output, indicating a false positive test failure. The faulty error message is specifically:ExampleGenerator.example() missing 1 required positional argument: 'try_predefined'.Log Output and Error Message:
🚨Something went wrong with Ersilia 🚨
Error message:
ExampleGenerator.example() missing 1 required positional argument: 'try_predefined'
Describe the steps to reproduce the behavior
No response
Operating environment
MacOS, Ersilia Virtual Environment
The text was updated successfully, but these errors were encountered: